On Mon, Jun 27, 2016 at 11:19 PM, Otto Moerbeek <o...@drijf.net> wrote:
> On Mon, Jun 27, 2016 at 09:33:40AM -0600, Ted Unangst wrote:
>
>> CVSROOT: /cvs
>> Module name: src
>> Changes by: t...@cvs.openbsd.org 2016/06/27 09:33:40
>>
>> Modified files:
>> lib/libc/stdlib: malloc.c
>>
>> Log message:
>> defer munmap to after unlocking malloc. this can (unfortunately) be an
>> expensive syscall, and we don't want to tie up other threads. there's no
>> need to hold the lock, so defer it to afterwards.
>> from Michael McConville
>> ok deraadt
>
> There's a race condtion here. The assignments to d_unmap_me and
> unmap_me_sz are not atomic. A thread leaving could call munmap(2) on
> partiallly assigned data.
Yes, two _MALLOC_LEAVE calls can occur without a _MALLOC_ENTRY between them.
T1 -> MALLOC_LOCK, do stuff
T2 -> block in MALLOC_LOCK
T1 -> MALLOC_LEAVE, MMAP()
unmap_me is still set, but to an already unmaped region
T2 -> return from MALLOC_LOCK, do stuff
T1 -> block in MALLOC_ENTRY
T2 -> MALLOC_LEAVE
this unmaps the same unmap_me from above, which may have
already been mapped by some other thread
MALLOC_LEAVE must clear unmap_me after copying the value into local
variables and before releasing the lock, MALLOC_ENTRY shouldn't touch
them at all.
However, doing this only for the unlock/lock around MMAP() seems weird
and probably wrong. If you don't take the MMAP() path, then
LEAVE/ENTRY won't be used, unmap_me won't be cleared, and _MUNMAP()
will degenerate back into munmap() holding the lock.
So this needs a clear invariant, perhaps
"umap_me is only altered or examined when the lock is held and is
always NULL when the malloc lock is release"
...and then to implement that do the full dance (transfer to local
variable, set to NULL, then munmap after unlock) on *every* unlock,
and maybe throw in an assert(umap_me == NULL) when locking.
Other invariants are possible, but that one seems sufficient to get
the desired optimization and simple to implement.
Philip Guenther
