Re: Bug report: Memory corrupion due to stale robust_list.head pointer

[Rich Felker <dalias@libc.org>]

On Wed, Sep 25, 2019 at 12:05:18PM +0200, d.dorau@avm.de wrote:
> Hello,
> 
> I recently came across a memory corruption in the member "tsd" of struct 
> pthread
> in a scenario where a pthread mutex is intentionally held during fork().
> I experienced this using the lastest release 1.1.23.
> 
> I found that during fork musl resets self->robust_list.pending and 
> self->robust_list.off,
> but not the robust_list.head. The stale pointer to the previously held and 
> reset
> mutex turned out to be the cause for the following corruption.
> 
> I therefore suggest to also reset the list head on fork as such:
> 
> --- a/src/process/fork.c.orig   2019-09-23 11:41:01.381626360 +0200
> +++ b/src/process/fork.c        2019-09-23 11:41:26.657819473 +0200
> @@ -27,6 +27,7 @@
>                 self->tid = __syscall(SYS_gettid);
>                 self->robust_list.off = 0;
>                 self->robust_list.pending = 0;
> +               self->robust_list.head = &self->robust_list.head;
>                 self->next = self->prev = self;
>                 __thread_list_lock = 0;
>                 libc.threads_minus_1 = 0;
> 
> 
> This resolves the issue.
> 
> I am very well aware of the fact that aquiring a mutex during fork and 
> re-initializing 
> in the child appears to result in undefined behaviour (as of 
> pthread_mutex_init(3posix))
> or to be controversial at least.
> 
> However I believe that it should't result in a memory corruption as a 
> result.

This is definitely undefined behavior, for exactly the types of reason
you encountered. I agree it's useful to limit the scope of memory
corruption that might occur on such invalid usage though.

Your patch is probably ok but before merging I'd like to take some
time to consider whether there are any valid usage cases it breaks (so
far, I don't think so) and whether any other easily-caught cases are
missed. For example, it is possible that the child attempts to use the
mutex in non-UB ways like locking or unlocking it, but those should
produce errors before even touching the robust list. In theory there's
a tid-reuse problem that could allow child to unlock a mutex that used
to be held by another thread in the parent, but (1) this can only be
hit if the child is doing non-AS-safe things after a MT fork, and (2)
musl already avoids this problem by using the robust list even for
non-robust mutexes. I don't think (2) is 100% complete in the case
where a MT program forks with non-pshared mutexes and the child makes
new threads, but this is very very far into UB territory.

> To reproduce I wrote a small example which triggers and shows the 
> curruption.
> It also contains a description of the program flow and memory corruption.
> 
> Please find it attached to this mail.
> 
> Please note that the routine to print the robust_list is hacked using 
> hardcoded
> offsets which are aimed at my 32-Bit platform.

Thanks for the detailed report.

Rich