Jeff King <peff@xxxxxxxx> 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
tr@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html