Re: [PATCH] fix use of pointer after free in unsetenv

[Alexander Cherepanov <ch3root@openwall.com>]

On 2016-01-04 10:42, Markus Wichmann wrote:
> On Mon, Jan 04, 2016 at 02:09:44AM +0300, Alexander Cherepanov wrote:
>> Hi!
>>
>> The code in [1] uses a pointer which was freed and hence has an
>> indeterminate value. Patch attached.
>>
>> [1] http://git.musl-libc.org/cgit/musl/tree/src/env/unsetenv.c#n23
>>
>
> What are you talking about? free() ends the lifetime of the object
> pointed to by the argument given.

Right, and C11, 6.2.4p2, adds: "The value of a pointer becomes 
indeterminate when the object it points to (or just past) reaches the 
end of its lifetime."

> However, after the free() only the
> pointer itself is used. It won't be dereferenced again, and instead will
> be immediately overwritten with a valid pointer. And while it is
> possible that the pointer becomes so invalid that even loading it causes
> undefined behavior, this doesn't happen on any of the supported systems
> (it might happen on i386 with segmentation, but Linux/i386, which is the
> only supported OS for musl, doesn't use segmentation).

You presume that the bits would be loaded that were there before free(). 
But this is not mandated by the C standard. A compiler is permitted to 
completely replace the value of the pointer with, e.g., NULL (as a 
hardening measure, to catch use-after-free) or just use NULL as the 
pointer's value in the first comparison in the loop (as an 
optimization). It's permitted by the rules of the C standard and makes 
the program faster and smaller (the whole "for" loop is eliminated), so 
why not?

IOW even if it's technically not undefined behavior (this could be 
debated) and not broken by current compilers it's wrong C, could be 
broken by future compilers, will trigger sanitizers/verifiers (I hope at 
least some of them will be able to catch this) etc.

> So it looks like unnecessary complexity to me to apply this patch.

It has its pros and cons. IMHO it's better to fix this problem (with my 
patch or in any other way) but that's just MHO and I appreciate that 
POVs vary.

-- 
Alexander Cherepanov