Re: [PATCH] Refactor Linux raise implementation (BZ#15368)

[Zack Weinberg <zackw at panix dot com>]

On Fri, Jun 17, 2016 at 2:43 PM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> This patch changes both the nptl and libc Linux raise implementation
> to avoid the issues described in BZ#15368.

It would be nice to have comments in these files which explain why it
is necessary to block all signals and why the cached values are not
safe to use.  The old comment that you deleted ("raise is async-safe
and could be called while fork/vfork temporarily invalidated the PID")
would be a good start.

> The strategy used is
> summarized in bug report first comment:
>
>  1. Block all signals (including internal NPTL ones);

The code appears to block all signals *except* the internal NPTL ones.
If I understand Rich's description of the problem correctly, that is
wrong.

> +static inline int
> +__libc_signal_block_all (const sigset_t *set)
> +{

Type-safety error here: the 'set' argument is written to and should
not be 'const'.  It compiles because INTERNAL_SYSCALL() erases types,
but the compiler would be entitled to assume that 'set' is unchanged
after the call.

> +static inline int
> +__libc_signal_block_app (const sigset_t *set)

Same type-safety error here.

> +  sigset_t set;
> +  __libc_signal_block_app (&set);
> +
> +  pid_t pid = __getpid ();

If I'm reading the code correctly, __getpid does *not* bypass the cache.

> +  pid_t tid = INLINE_SYSCALL (gettid, 0);

INTERNAL_SYSCALL() here too?  gettid() can't fail, so there's no point
generating the code to potentially set errno.

zw