Re: [PATCH] Fix maybe-uninitialized warnings found by gcc 9 -flto

From: Jeff King <peff@peff.net>
Date: Sep 5, 2019, 17:53 UTC
Message-ID: <20190905175304.GA23663@sigill.intra.peff.net>
In-Reply-To: <6c6c4e71-d9e5-1ad4-74db-12c323da42f7@web.de>

On Thu, Sep 05, 2019 at 07:08:49PM +0200, René Scharfe wrote:

> Anyway, my points are that simply initializing might not always be the
> best fix, and that more context would help reviewers of such a patch,
> but only if functions are reasonably short and it's not necessary to
> follow the rabbit into a call chain hole.

I started looking at these, too, and came to the same conclusion. Some of these are pointing to real bugs, and just silencing the warnings misses the opportunity to find them.

I'll comment on the ones I did look at.

Show 11 quoted lines

> Further context:
> 
> 	ident_line = find_commit_header(buffer, "author", &ident_len);
> 
> 	if (split_ident_line(&id, ident_line, ident_len) < 0)
> 		die(_("invalid ident line: %.*s"), (int)ident_len, ident_line);
> 
> find_commit_header() can return NULL.  split_ident_line() won't handle
> that well.  So I think what's missing here is a NULL check.  If the
> compiler is smart enough then that should silence the initialization
> warning.

Yeah, this one is a segfault waiting to happen, I think, if the author line is missing.

Show 16 quoted lines

> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index 76ce906946..d0c03b0e9b 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
> > @@ -1171,7 +1171,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
> >  {
> >  	struct packed_git *found_pack = NULL;
> >  	off_t found_offset = 0;
> > -	uint32_t index_pos;
> > +	uint32_t index_pos = 0;
> [...]
> So if the packing list is empty then it returns NULL without setting
> index_pos.  Hmm.  It does set it in all other cases, no matter if oid is
> found or not.  Is it really a good idea to make that exception?  I
> suspect always setting index_pos here would silence the compiler as well
> and fix the issue closer to its root.

Yeah, this is a weird one. That index_pos is actually a position in the current hash table. And in the first object we see in pack-objects' input, we definitely _do_ end up with a nonsense index_pos, which then gets passed to packlist_alloc().

But since we also need to grow the hash table during that allocation, we don't use index_pos at all. So setting index_pos to something known like "0" is kind of weird, in that we don't have a hash position at all (the table doesn't exist!). But it's probably the least bad thing. If we do that, it should happen in packlist_find().

I have to agree this whole "passing around index_pos" thing looks complicated. It seems like we're just saving ourselves one hash lookup on insertion (and it's not even an expensive hash, since we're reusing the oid).

I suspect the whole thing would also be a lot simpler using one of our existing hash implementations.

> Didn't check the other cases.

The only other one I looked at was:

>> int cmd__read_cache(int argc, const char **argv)
>> {
>>-       int i, cnt = 1, namelen;
>>+       int i, cnt = 1, namelen = 0;

I actually saw this one the other day, because it triggered for me when compiling with SANITIZE=address. AFAICT it's a false positive. "name" is always NULL unless skip_prefix() returns true, in which case we always set "namelen". And we only look at "namelen" if "name" is non-NULL.

This one doesn't even require LTO, because skip_prefix() is an inline function. I'm not sure why the compiler gets confused here. I don't mind initializing namelen to 0 to silence it, though (we already set name to NULL, so this would just match).

-Peff