[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <568A5E64.2020105@openwall.com>
Date: Mon, 4 Jan 2016 14:58:28 +0300
From: Alexander Cherepanov <ch3root@...nwall.com>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] fix use of pointer after free in unsetenv
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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
