Jeff King <> writes:

> This code is weird to follow because of the fall-throughs. I do not
> think you have introduced any bugs with your patch, but it seems weird
> to me that we set the offset at the top of the hunk. If we hit the
> conditional in the bottom half, we do actually increment storer.seen,
> but only _after_ having overwritten the value from above (with the same
> value, no less).
> But if we do not follow that code path, we may end up here:
>> @@ -1272,6 +1275,9 @@ static int store_aux(const char *key, const char 
>> *value, void *cb)
>>                      if (strrchr(key, '.') - key == store.baselen &&
>>                            !strncmp(key, store.key, store.baselen)) {
>>                                      store.state = SECTION_SEEN;
>> +                                    ALLOC_GROW(store.offset,
>> +                                               store.seen+1,
>> +                                               store.offset_alloc);
>>                                      store.offset[store.seen] = 
>> cf->do_ftell(cf);
>>                      }
>>              }
> where we overwrite it again, but do not update store.seen. Or we may
> trigger neither, and leave the function with our offset stored, but
> store.seen not incremented.

It's doubly strange that we write in this hunk without any protection
against overflow.  I was too lazy to think about it long enough to come
up with a possible example that triggers this, and instead just put in
the defensive ALLOC_GROW().  But if you can trigger it, it will probably
cause the algorithm to go off the rails because it overwrote store.state
and possibly even store.seen.

Thomas Rast
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to
More majordomo info at

Reply via email to