Re: [PATCH] config: arbitrary number of matches for --unset and --replace-all

From: Thomas Rast <tr@thomasrast.ch>
Date: Nov 14, 2013, 20:24 UTC
Message-ID: <87zjp6loiz.fsf@linux-k42r.v.cablecom.net>
In-Reply-To: <20131114083747.GD16327@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 23 quoted lines

> 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@thomasrast.ch