Bug 15368

Summary: raise() is not async-signal-safe
Product: glibc Reporter: Rich Felker <bugdal>
Component: nptlAssignee: Not yet assigned to anyone <unassigned>
Status: RESOLVED FIXED    
Severity: normal CC: adhemerval.zanella, carlos, drepper.fsp, jeremip11
Priority: P2 Flags: fweimer: security-
Version: unspecified   
Target Milestone: 2.24   
Host: Target:
Build: Last reconfirmed:

Description Rich Felker 2013-04-15 02:50:54 UTC
NPTL's raise() is implemented as a call to pthread_kill(pthread_self(), sig); this implementation is invalid because pthread_kill is not async-signal-safe. In particular, if a signal handler forks during raise, the the thread in the parent could receive the signal twice, once from itself and once from the child. Or, if raise() is called from a signal handler while fork() has temporarily modified the tid/pid in TLS, raise() may fail to do anything. Other incorrect behavior may also be possible too; I have not analyzed the issue in detail.

The (only) correct implementation of raise() for multi-threaded programs is:

1. Block all signals.
2. Get pid and tid from the kernel via syscalls.
3. Make the tgkill syscall.
4. Restore the old signal mask.

Blocking signals is required to make obtaining the tid/pid atomic with sending the signal. Anything else fails to meet the async-signal safety requirements.
Comment 1 Carlos O'Donell 2016-04-15 04:50:55 UTC
This issue still applies in general.

The point about double signal delivery is valid (and worrisome).

The issues about raise() doing nothing are not correct since raise() coordinates with fork() and vfork() over the use of -PID. However, clone() sets PID/TID to -1/-1 and that will certainly break a raise() in a signal handler delivered to the child of the clone so that's a bug (one which will be fixed soon probably by removing setting pid/tid to -1/-1 in clone).
Comment 2 cvs-commit@gcc.gnu.org 2016-07-13 16:10:35 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, master has been updated
       via  2ac88eecc57ff00e0b5ff803ebcc3465d2d640dd (commit)
      from  e15eaa8f335ebfd565ab7752c64f3415d427d9b2 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=2ac88eecc57ff00e0b5ff803ebcc3465d2d640dd

commit 2ac88eecc57ff00e0b5ff803ebcc3465d2d640dd
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date:   Fri Apr 22 09:25:20 2016 -0300

    Refactor Linux raise implementation (BZ#15368)
    
    This patch changes both the nptl and libc Linux raise implementation
    to avoid the issues described in BZ#15368.  The strategy used is
    summarized in bug report first comment:
    
     1. Block all signals (including internal NPTL ones);
     2. Get pid and tid directly from syscall (not relying on cached
        values);
     3. Call tgkill;
     4. Restore old signal mask.
    
    Tested on x86_64 and i686.
    
    	[BZ #15368]
    	* sysdeps/unix/sysv/linux/nptl-signals.h
    	(__nptl_clear_internal_signals): New function.
    	(__libc_signal_block_all): Likewise.
    	(__libc_signal_block_app): Likewise.
    	(__libc_signal_restore_set): Likewise.
    	* sysdeps/unix/sysv/linux/pt-raise.c (raise): Use Linux raise.c
    	implementation.
    	* sysdeps/unix/sysv/linux/raise.c (raise): Reimplement to not use
    	the cached pid/tid value in pthread structure.

-----------------------------------------------------------------------

Summary of changes:
 ChangeLog                              |   13 +++++++
 sysdeps/unix/sysv/linux/nptl-signals.h |   41 +++++++++++++++++++++
 sysdeps/unix/sysv/linux/pt-raise.c     |   23 ++----------
 sysdeps/unix/sysv/linux/raise.c        |   63 +++++++++++++++++---------------
 4 files changed, 90 insertions(+), 50 deletions(-)
Comment 3 Adhemerval Zanella 2016-07-14 08:16:03 UTC
Fixed by 2ac88eecc57ff00e0b5ff803ebcc3465d2d640dd.
Comment 4 cvs-commit@gcc.gnu.org 2018-03-26 12:33:39 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU C Library master sources".

The branch, zack/wip-check-localplt-2 has been created
        at  c6c5c2887d8b6883ee69a1e183119f2be02e660f (commit)

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=c6c5c2887d8b6883ee69a1e183119f2be02e660f

commit c6c5c2887d8b6883ee69a1e183119f2be02e660f
Author: Zack Weinberg <zackw@panix.com>
Date:   Sun Mar 18 17:01:06 2018 -0400

    WIP finer-grained, more aggressive local PLT call check

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=9ea49e16c79bd2acd0d0648ca0163f26dd1c3dae

commit 9ea49e16c79bd2acd0d0648ca0163f26dd1c3dae
Author: Zack Weinberg <zackw@panix.com>
Date:   Fri Mar 23 09:16:59 2018 -0400

    [Bug 15368] Move pthread_kill to libc and use it to implement raise.
    
    The fix for bug #15368 was unnecessarily Linux-specific.  To recap,
    POSIX specifies raise to be async-signal-safe, but also specifies it
    to be equivalent to pthread_kill(pthread_self(), sig), which is not
    an async-signal-safe sequence of operations; a signal handler could
    run in between pthread_self and pthread_kill, and do something (such
    as calling fork, which is also async-signal-safe) that would invalidate
    the thread descriptor.  This is even true in the hypothetical case of
    a port that doesn't implement multithreading: kill(getpid(), sig) will
    fire the signal twice if a signal handler runs in between, calls fork,
    and then returns on both sides of the fork.  I don't see anything in
    the standards to forbid that.
    
    The Linux-specific fix was to override the definitions of raise in
    both libpthread and libc to the same unitary function that blocks
    signals, retrieves TID and PID directly from the kernel, calls tgkill,
    and only then unblocks signals.  This patch generalizes that to any
    port: pthread_kill is moved from libpthread to libc, with a forwarding
    stub left behind.  The definition of raise in libpthread is also
    replaced with a forwarding stub.  The Linux-specific definition of
    raise is deleted; those ports will now use sysdeps/pthread/raise.c,
    which blocks signals first, then calls pthread_self and pthread_kill,
    and then unblocks signals.  Similarly, sysdeps/posix/raise.c (which
    would be used on a port that didn't implement multithreading) blocks
    signals, calls getpid and kill, and then unblocks signals.  Thus,
    ports need only implement the primitives correctly and do not need to
    worry about making raise async-signal-safe.
    
    The only wrinkle was that up till now, we did not bother initializing
    the ->tid field of the initial thread's descriptor unless libpthread
    was loaded; now that raise calls pthread_kill even in a single-
    threaded environment, that won't fly.  This is abstractly easy to fix;
    the tricky part was figuring out _where_ to put the calls (two of
    them, as it happens) to __pthread_initialize_pids, and I'd appreciate
    careful eyes on those changes.
    
    You might be wondering why it's safe to rely on the TID in the thread
    descriptor, rather than calling gettid directly.  Since all signals
    are blocked from before calling pthread_self until after pthread_kill
    uses the TID to call tgkill, the question is whether some _other_
    thread could do something that would invalidate the calling thread's
    descriptor, and I believe there is no such thing.
    
    While I was at it I fixed another bug: raise was returning an error
    code on failure (like pthread_kill does) instead of setting errno as
    specified.  This is user-visible but I don't think it's worth recording
    as a fixed bug, nobody bothers checking whether raise failed anyway.
    
    	* nptl/pt-raise.c
    	* sysdeps/unix/sysv/linux/pt-raise.c
    	* sysdeps/unix/sysv/linux/raise.c:
    	Remove file.
    
    	* sysdeps/unix/sysv/linux/pthread_kill.c: Use __is_internal_signal
    	to check for forbidden signals.  Use INTERNAL_SYSCALL_CALL to call
    	getpid.  Provide __libc_pthread_kill, with __pthread_kill as
    	strong alias and pthread_kill as weak alias.
    
    	* sysdeps/posix/raise.c: Block signals around the calls to
    	__getpid and __kill.  Provide __libc_raise, with raise as strong
    	alias, libc_hidden_def for raise, and gsignal as weak alias.
    	* sysdeps/pthread/raise.c: New file.  Implement by blocking
    	signals, calling pthread_self and pthread_kill, and then
    	unblocking signals again.  Provide same symbols as above.
    
    	* sysdeps/generic/internal-signals.h: Define all of the same
    	functions that sysdeps/unix/sysv/linux/internal-signals.h does,
    	with sensible default definitions.
    	* sysdeps/unix/sysv/linux/internal-signals.h: Clarify comments.
    
    	* nptl/pthread_kill.c: Define __libc_pthread_kill, with
    	__pthread_kill as strong alias and pthread_kill as weak alias.
    	* nptl/pthread_self.c: Define __pthread_self, with
    	pthread_self as weak alias.
    	* signal/raise.c: Define __libc_raise, with raise as strong alias,
    	libc_hidden_def for raise, and gsignal as weak alias.
    
    	* nptl/Makefile: Move pthread_kill from libpthread-routines to
    	routines.  Remove pt-raise from libpthread-routines.
    	* nptl/Versions (libc/GLIBC_2.28): Add pthread_kill.
    	(libc/GLIBC_PRIVATE): Add __libc_pthread_kill and __libc_raise.
    	* sysdeps/generic/pt-compat-stubs.S: Add stubs for raise and
    	pthread_kill.
    
    	* nptl/nptl-init.c (__pthread_initialize_minimal_internal):
    	Don't call __pthread_initialize_pids here.
    	* csu/libc-tls.c (__libc_setup_tls):
            Call __pthread_initialize_pids after all other setup.
    	* elf/rtld.c (init_tls): Likewise.
    
    	* include/pthreadP.h: New forwarder.
    	* include/pthread.h: Add multiple inclusion guard.  Declare
    	__pthread_self.
    	* include/signal.h: Declare __pthread_kill.
    
    	* sysdeps/**/libc.abilist (GLIBC_2.28): Add pthread_kill.

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=3d8eb8099425ae4f474e97082e04784c2984ec48

commit 3d8eb8099425ae4f474e97082e04784c2984ec48
Author: Zack Weinberg <zackw@panix.com>
Date:   Fri Mar 23 16:17:26 2018 -0400

    WIP no duplicate function definitions in pthread

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=65a7c4c9272f1b14a2b87733bf4dbb564c3dd1d2

commit 65a7c4c9272f1b14a2b87733bf4dbb564c3dd1d2
Author: Zack Weinberg <zackw@panix.com>
Date:   Sun Mar 18 17:12:42 2018 -0400

    RFC: Introduce pt-compat-stubs and use it to replace pt-vfork.
    
    I am looking into the possibility of eliminating all of the duplicate
    function definitions from libpthread, replacing them with
    properly-tagged weak compatibility symbols that just call the
    definition in libc.  Because one of the duplicated functions is vfork,
    the calls to libc absolutely must reuse the stack frame (a "sibcall"
    in GCC internals jargon), and on several important targets, GCC does
    not implement sibcalls, or only implements them for intra-module
    calls.  But we only need to implement a single special case,
    sibcalling a function with exactly the same signature, from
    immediately after the caller's own entry point; so doing it by hand in
    assembly language is not a crazy notion.  I believe I have managed to
    turn the trick for all currently-supported targets.  This patch just
    converts the existing vfork stub, so that review can focus on the new
    sysdep.h SIBCALL macros.
    
    	* sysdeps/generic/pt-compat-stubs.S: New file.
    	* nptl/Makefile (libpthread-routines): Remove pt-vfork, add
    	pt-compat-stubs.
    	(libpthread-shared-only-routines): Add pt-compat-stubs.
    	* posix/vfork.c: Define __libc_vfork as well as __vfork and vfork.
    
    	* sysdeps/generic/sysdep.h (SIBCALL): New macro to perform
    	sibling calls; the generic definition errors out if used.
    	* sysdeps/aarch64/sysdep.h, sysdeps/arm/sysdep.h
    	* sysdeps/hppa/sysdep.h, sysdeps/ia64/sysdep.h
    	* sysdeps/m68k/sysdep.h, sysdeps/microblaze/sysdep.h
    	* sysdeps/nios2/sysdep.h, sysdeps/powerpc/powerpc32/sysdep.h
    	* sysdeps/powerpc/powerpc64/sysdep.h, sysdeps/s390/s390-32/sysdep.h
    	* sysdeps/s390/s390-64/sysdep.h, sysdeps/tile/sysdep.h
    	* sysdeps/unix/alpha/sysdep.h, sysdeps/unix/mips/mips32/sysdep.h
    	* sysdeps/unix/mips/mips64/n32/sysdep.h
    	* sysdeps/unix/mips/mips64/n64/sysdep.h
    	* sysdeps/unix/sysv/linux/riscv/sysdep.h
    	* sysdeps/x86/sysdep.h
    	Provide appropriate architecture-specific definitions of
    	SIBCALL and, if necessary, SIBCALL_ENTRY.
    
    	* nptl/pt-vfork.c
    	* sysdeps/unix/sysv/linux/aarch64/pt-vfork.c
    	* sysdeps/unix/sysv/linux/m68k/pt-vfork.c
    	* sysdeps/unix/sysv/linux/tile/pt-vfork.c
    	* sysdeps/unix/sysv/linux/alpha/pt-vfork.S
    	* sysdeps/unix/sysv/linux/hppa/pt-vfork.S
    	* sysdeps/unix/sysv/linux/ia64/pt-vfork.S
    	* sysdeps/unix/sysv/linux/microblaze/pt-vfork.S
    	* sysdeps/unix/sysv/linux/mips/pt-vfork.S
    	* sysdeps/unix/sysv/linux/riscv/pt-vfork.S
    	* sysdeps/unix/sysv/linux/s390/pt-vfork.S
    	* sysdeps/unix/sysv/linux/sh/pt-vfork.S
    	* sysdeps/unix/sysv/linux/sparc/pt-vfork.S
    	Remove file.

-----------------------------------------------------------------------