fork, set*id(synccall), cancellation -- nasty interaction

fork, set*id(synccall), cancellation -- nasty interaction

[Rich Felker]

I've run across a nasty set of race conditions I'm trying to solve:

1. __synccall, needed for multi-threaded set*id, needs to obtain a
lock that prevents thread creation and other things. However, setuid
and setgid are specified to be async-signal-safe. They will presently
hang if called from a signal handler that interrupted pthread_create
or several other functions.

2. When forking, the calling thread's pid/tid change, and there is a
window of time during which the child process has the wrong pid/tid in
its thread descriptor. Considering that the next version of POSIX will
remove fork from the list of async-signal-safe functions, and glibc's
fork is already non-async-signal-safe, it seems that we could just go
ahead and consider it unsafe. There are only two async-signal-safe
code paths that care about the pid/tid in the thread descriptor:
__synccall (called from setuid/setgid) and the cancellation signal
handler (called asynchronously). We could prevent these from ever
seeing wrong values in the thread descriptor by blocking signals
before making the fork syscall and restoring them afterwards. This is
probably the correct solution. However, there is still at least one
other problem:

3. If thread B forks while thread A is in the middle of starting a
__synccall operation, pthread_create, or anything else dealing with
the lock mentioned in point 1 above, the child process will inherit an
inconstent state. Normally this would not be a big deal since
multi-threaded programs are forbidden from doing anything
async-signal-unsafe after forking. However, setuid and setgid are
specified to be async-signal-safe, but cannot handle the inconsistent
state. The case where they're called after fork returns in the child
is not a problem, since libc.threads_minus_1 will be 0 and the
__synccall logic will not be used, but if fork was called from a
signal handler that interrupted setuid() after it was determined that
__synccall is needed but before __synccall actually began, we still
hit the trouble case. Perhaps this can be solved though: if __synccall
first blocks all signals except its own internal signal (blocking its
own before taking the lock would lead to deadlock) then tests
libc.threads_minus_1 before proceeding, it could determine that the
process has switched to single-threaded mode and avoid the whole
__synccall logic. Due to signals being blocked, there is no way fork
could be called in this window.

The hardest problem seems to be #1, and the only immediate solution I
see is just making the operation fail if the lock is not available.
This technically makes it async-signal-safe, but it's an undesirable
failure case...

Anyway, I'm mainly posting this to have a good record of the issues,
but I would also be happy to hear any ideas towards fixing them.

Rich

Re: fork, set*id(synccall), cancellation -- nasty interaction

[Rich Felker]

On Fri, Apr 26, 2013 at 01:27:22PM -0400, Rich Felker wrote:
> I've run across a nasty set of race conditions I'm trying to solve:
> 
> 1. __synccall, needed for multi-threaded set*id, needs to obtain a
> lock that prevents thread creation and other things. However, setuid
> and setgid are specified to be async-signal-safe. They will presently
> hang if called from a signal handler that interrupted pthread_create
> or several other functions.

I have a solution, but it's ugly and would decrease thread creation
performance by more than 10%. For static linked programs, the penalty
could be eliminated if __synccall is not used, but adding the logic to
do this would make the code much uglier.

The concept is fairly simple:

1. Fully protect the thread count by the "ptc" rwlock, so that if
another thread holds __inhibit_ptc(), the thread count can neither
increase nor decrease.

2. Prevent any calls to __synccall from interrupting code paths that
hold the "ptc" lock.

However, the requirements this translates into are:

1. pthread_create must block application signals unconditionally.
Right now it only does so in the special case of applying scheduling
changes to the new thread.

2. pthread_exit must perform the following acrobatics: First, block
application signals. Then, __acquire_ptc(). Then, block all signals,
then decrement the thread count and __release_ptc(). This adds both an
extra lock/unlock step and a second sigprocmask syscall to the exit
procedure. The two-step signal blocking is needed because, if all
signals were blocked at the time of the __acquire_ptc() call, it could
deadlock with another thread calling __synccall that had already
successfully performed __inhibit_ptc() and begun the broadcast.

3. The other user of __inhibit_ptc(), dlopen, would either need to
block signals for its duration, or the "ptc" rwlock could be replaced
by a two-way symmetric lock (allowing multiple 'readers' or multiple
'writers' but not both).

Basically, it's doable, but ugly. I'm still looking for better
solutions...

Rich