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.
> 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.
> > 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
