Re: [PATCH v2] merge: fix cache_entry use-after-free

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 13, 2015, 21:05 UTC
Message-ID: <xmqqio6aa38e.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1444764167.7234.14.camel@twopensource.com>

David Turner <dturner@twopensource.com> writes:

Show 10 quoted lines

>> This one smelled iffy.  I think it is safe because the caller does
>> not look at src[] other than src[0] after this function returns, and
>> this setting to NULL happens only when o->merge is set to 1, so I do
>> not think this is buggy, but at the same time I do not think setting
>> to NULL is necessary.
>> 
>> Other than that, looks nice.  Thanks.
>
> I like to set a pointer to NULL after I free the thing pointed to by it,
> because it helps make use-after-free bugs easier to detect.

Yes, but it also helps unintended bugs to creep in if done blindly, and that is why I vetted what happens in the codepath of the caller of this function after src[] entries are NULLed (the caller could be expecting to do things only to NULLed fields, for example, in which case clearing them like this patch would have changed the behaviour of the caller after this function returns).

Thanks.