Introduce sys_sigprocmask and sys_sigaction.

Introduce sys_sigprocmask and sys_sigaction.

This is preparation of nacl_helper_nonsfi's secompbpf sandbox
implementation.
PNaCl toolchain's signal ABI is incompatible with Linux's.
For example, the length of sigset_t is shoter than Linux's.
(Android has also same problem).
siginfo_t has different memory layout.
Some signal numbers, including SIGBUS, SIGCHLD and SIGSYS,
or some signal flags are different.
This CL fills the gap, by introducing linux_signal.h and
two syscalls, sys_sigprocmask and sys_sigaction.

Also, as signal.h provides ucontext_t, but PNaCl toolchain
does not provides ucontext_t, this CL re-use the
android_ucontext.h (by renaming it to linux_ucontext.h).

TEST=Ran bots.
BUG=358465
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_chromium_trusty32_rel,linux_arm

Committed: https://crrev.com/3a315e3246a5191cee27216c09e669ea2d6c33d3
Cr-Commit-Position: refs/heads/master@{#325634}

[hidehiko, 2015-04-10 18:26:39]

hidehiko@chromium.org changed reviewers:
+ mdempsky@chromium.org

[hidehiko, 2015-04-10 18:26:40]

Could you take a look?

- hidehiko

[hidehiko, 2015-04-10 18:26:49]

Patchset #1 (id:1) has been deleted

[hidehiko, 2015-04-10 18:26:58]

Patchset #1 (id:20001) has been deleted

[mdempsky, 2015-04-13 18:55:07]

First pass of feedback.

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
File sandbox/linux/seccomp-bpf/die.cc (right):

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
sandbox/linux/seccomp-bpf/die.cc:40: sigaddset(&sa.sa_mask, LINUX_SIGSEGV);
With POSIX sigaction(), SIGSEGV would be masked automatically since you didn't
specify SA_NODEFER.  Does Linux's sigaction() system call push that
responsibility into userland?

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
File sandbox/linux/seccomp-bpf/trap.h (right):

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
sandbox/linux/seccomp-bpf/trap.h:8: #include <signal.h>
You can probably remove this header now?

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
File sandbox/linux/services/syscall_wrappers.cc (right):

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
sandbox/linux/services/syscall_wrappers.cc:142: int how, const sigset_t* set,
decltype(nullptr) oldset) {
Nit: This line breaking seems odd.  Can you please run "git cl format"?

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
sandbox/linux/services/syscall_wrappers.cc:143: // In some toolchain (in
particular ANDROID and PNaCl toolchain),
Nit: Just "Android".

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
sandbox/linux/services/syscall_wrappers.cc:146: std::memcpy(&linux_value, set,
std::min(sizeof(sigset_t), sizeof(uint64_t)));
Nit: You need <cstring> for std::memcpy (or <string.h> for memcpy, which seems
more common in Chromium) and <algorithm> for std::min.

I'm also a little nervous about just using memcpy like this, because it makes
assumptions about how the libc defines sigset_t.  Can you add a unit test to
make sure this is safe?  E.g., just create a sigset_t, add a few signals with
sigaddset, memcpy to a uint64_t, and then check that it has the expected value.

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
sandbox/linux/services/syscall_wrappers.cc:193: std::memcpy(&oldact->sa_mask,
&kernel_oldact.sa_mask,
I think you also want to memset &oldact->sa_mask to 0 first in case
sizeof(sa_mask) > sizeof(uint64_t)?

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/system_he...
File sandbox/linux/system_headers/arm_linux_ucontext.h (right):

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/system_he...
sandbox/linux/system_headers/arm_linux_ucontext.h:38: typedef struct sigaltstack
{
Nit: clang-format

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/system_he...
File sandbox/linux/system_headers/i386_linux_ucontext.h (right):

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/system_he...
sandbox/linux/system_headers/i386_linux_ucontext.h:80: // Android and PNaCl
toolchain's sigset_t has only 32 bits, though Linux
Does this mean __fpregs_mem was previously wrong on Android i386?

[hidehiko, 2015-04-14 06:02:27]

Patchset #3 (id:80001) has been deleted

[hidehiko, 2015-04-14 06:02:37]

Patchset #3 (id:100001) has been deleted

[hidehiko, 2015-04-14 11:56:41]

Thank you for review!
PTAL.
- hidehiko

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
File sandbox/linux/seccomp-bpf/die.cc (right):

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
sandbox/linux/seccomp-bpf/die.cc:40: sigaddset(&sa.sa_mask, LINUX_SIGSEGV);
On 2015/04/13 18:55:07, mdempsky wrote:
> With POSIX sigaction(), SIGSEGV would be masked automatically since you didn't
> specify SA_NODEFER.  Does Linux's sigaction() system call push that
> responsibility into userland?

IIUC, SIGSEGV is masked in the handler invocation (if we do not set SA_NODEFER),
right? So, I didn't think it is a problem here...?

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
File sandbox/linux/seccomp-bpf/trap.h (right):

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
sandbox/linux/seccomp-bpf/trap.h:8: #include <signal.h>
On 2015/04/13 18:55:07, mdempsky wrote:
> You can probably remove this header now?

Done.

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
File sandbox/linux/services/syscall_wrappers.cc (right):

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
sandbox/linux/services/syscall_wrappers.cc:142: int how, const sigset_t* set,
decltype(nullptr) oldset) {
On 2015/04/13 18:55:07, mdempsky wrote:
> Nit: This line breaking seems odd.  Can you please run "git cl format"?

Done.

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
sandbox/linux/services/syscall_wrappers.cc:143: // In some toolchain (in
particular ANDROID and PNaCl toolchain),
On 2015/04/13 18:55:07, mdempsky wrote:
> Nit: Just "Android".

Done.

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
sandbox/linux/services/syscall_wrappers.cc:146: std::memcpy(&linux_value, set,
std::min(sizeof(sigset_t), sizeof(uint64_t)));
On 2015/04/13 18:55:07, mdempsky wrote:
> Nit: You need <cstring> for std::memcpy (or <string.h> for memcpy, which seems
> more common in Chromium) and <algorithm> for std::min.
> 
> I'm also a little nervous about just using memcpy like this, because it makes
> assumptions about how the libc defines sigset_t.  Can you add a unit test to
> make sure this is safe?  E.g., just create a sigset_t, add a few signals with
> sigaddset, memcpy to a uint64_t, and then check that it has the expected
value.

Done.

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
sandbox/linux/services/syscall_wrappers.cc:193: std::memcpy(&oldact->sa_mask,
&kernel_oldact.sa_mask,
On 2015/04/13 18:55:07, mdempsky wrote:
> I think you also want to memset &oldact->sa_mask to 0 first in case
> sizeof(sa_mask) > sizeof(uint64_t)?

Good catch. Done.

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/system_he...
File sandbox/linux/system_headers/arm_linux_ucontext.h (right):

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/system_he...
sandbox/linux/system_headers/arm_linux_ucontext.h:38: typedef struct sigaltstack
{
On 2015/04/13 18:55:07, mdempsky wrote:
> Nit: clang-format

Done.

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/system_he...
File sandbox/linux/system_headers/i386_linux_ucontext.h (right):

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/system_he...
sandbox/linux/system_headers/i386_linux_ucontext.h:80: // Android and PNaCl
toolchain's sigset_t has only 32 bits, though Linux
On 2015/04/13 18:55:07, mdempsky wrote:
> Does this mean __fpregs_mem was previously wrong on Android i386?

IIUC, yes. However, it wasn't a problem, because sandbox does not use it.

[hidehiko, 2015-04-16 00:44:27]

On 2015/04/14 11:56:41, hidehiko wrote:
> Thank you for review!
> PTAL.
> - hidehiko
> 
>
https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
> File sandbox/linux/seccomp-bpf/die.cc (right):
> 
>
https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
> sandbox/linux/seccomp-bpf/die.cc:40: sigaddset(&sa.sa_mask, LINUX_SIGSEGV);
> On 2015/04/13 18:55:07, mdempsky wrote:
> > With POSIX sigaction(), SIGSEGV would be masked automatically since you
didn't
> > specify SA_NODEFER.  Does Linux's sigaction() system call push that
> > responsibility into userland?
> 
> IIUC, SIGSEGV is masked in the handler invocation (if we do not set
SA_NODEFER),
> right? So, I didn't think it is a problem here...?
> 
>
https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
> File sandbox/linux/seccomp-bpf/trap.h (right):
> 
>
https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
> sandbox/linux/seccomp-bpf/trap.h:8: #include <signal.h>
> On 2015/04/13 18:55:07, mdempsky wrote:
> > You can probably remove this header now?
> 
> Done.
> 
>
https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
> File sandbox/linux/services/syscall_wrappers.cc (right):
> 
>
https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
> sandbox/linux/services/syscall_wrappers.cc:142: int how, const sigset_t* set,
> decltype(nullptr) oldset) {
> On 2015/04/13 18:55:07, mdempsky wrote:
> > Nit: This line breaking seems odd.  Can you please run "git cl format"?
> 
> Done.
> 
>
https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
> sandbox/linux/services/syscall_wrappers.cc:143: // In some toolchain (in
> particular ANDROID and PNaCl toolchain),
> On 2015/04/13 18:55:07, mdempsky wrote:
> > Nit: Just "Android".
> 
> Done.
> 
>
https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
> sandbox/linux/services/syscall_wrappers.cc:146: std::memcpy(&linux_value, set,
> std::min(sizeof(sigset_t), sizeof(uint64_t)));
> On 2015/04/13 18:55:07, mdempsky wrote:
> > Nit: You need <cstring> for std::memcpy (or <string.h> for memcpy, which
seems
> > more common in Chromium) and <algorithm> for std::min.
> > 
> > I'm also a little nervous about just using memcpy like this, because it
makes
> > assumptions about how the libc defines sigset_t.  Can you add a unit test to
> > make sure this is safe?  E.g., just create a sigset_t, add a few signals
with
> > sigaddset, memcpy to a uint64_t, and then check that it has the expected
> value.
> 
> Done.
> 
>
https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/services/...
> sandbox/linux/services/syscall_wrappers.cc:193: std::memcpy(&oldact->sa_mask,
> &kernel_oldact.sa_mask,
> On 2015/04/13 18:55:07, mdempsky wrote:
> > I think you also want to memset &oldact->sa_mask to 0 first in case
> > sizeof(sa_mask) > sizeof(uint64_t)?
> 
> Good catch. Done.
> 
>
https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/system_he...
> File sandbox/linux/system_headers/arm_linux_ucontext.h (right):
> 
>
https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/system_he...
> sandbox/linux/system_headers/arm_linux_ucontext.h:38: typedef struct
sigaltstack
> {
> On 2015/04/13 18:55:07, mdempsky wrote:
> > Nit: clang-format
> 
> Done.
> 
>
https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/system_he...
> File sandbox/linux/system_headers/i386_linux_ucontext.h (right):
> 
>
https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/system_he...
> sandbox/linux/system_headers/i386_linux_ucontext.h:80: // Android and PNaCl
> toolchain's sigset_t has only 32 bits, though Linux
> On 2015/04/13 18:55:07, mdempsky wrote:
> > Does this mean __fpregs_mem was previously wrong on Android i386?
> 
> IIUC, yes. However, it wasn't a problem, because sandbox does not use it.

Matthew, friendly ping?

[mdempsky, 2015-04-17 04:36:22]

LGTM

Sorry for the delay!

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
File sandbox/linux/seccomp-bpf/die.cc (right):

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
sandbox/linux/seccomp-bpf/die.cc:40: sigaddset(&sa.sa_mask, LINUX_SIGSEGV);
On 2015/04/14 11:56:40, hidehiko wrote:
> On 2015/04/13 18:55:07, mdempsky wrote:
> > With POSIX sigaction(), SIGSEGV would be masked automatically since you
didn't
> > specify SA_NODEFER.  Does Linux's sigaction() system call push that
> > responsibility into userland?
> 
> IIUC, SIGSEGV is masked in the handler invocation (if we do not set
SA_NODEFER),
> right? So, I didn't think it is a problem here...?

Sorry, what I meant was:

  1. Setting sa_mask is only meaningful if you set a signal handler; I don't
think setting it is meaningful here because you're using SIG_DFL.
  2. Even if we were registering a handler here, then because we're not setting
SA_NODEFER, SIGSEGV would be masked anyway.

So I think this sigaddset is unnecessary (although not harmful either).

[hidehiko, 2015-04-17 06:18:41]

Thank you for review! Submitting.

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
File sandbox/linux/seccomp-bpf/die.cc (right):

https://codereview.chromium.org/1077143002/diff/40001/sandbox/linux/seccomp-b...
sandbox/linux/seccomp-bpf/die.cc:40: sigaddset(&sa.sa_mask, LINUX_SIGSEGV);
On 2015/04/17 04:36:22, mdempsky wrote:
> On 2015/04/14 11:56:40, hidehiko wrote:
> > On 2015/04/13 18:55:07, mdempsky wrote:
> > > With POSIX sigaction(), SIGSEGV would be masked automatically since you
> didn't
> > > specify SA_NODEFER.  Does Linux's sigaction() system call push that
> > > responsibility into userland?
> > 
> > IIUC, SIGSEGV is masked in the handler invocation (if we do not set
> SA_NODEFER),
> > right? So, I didn't think it is a problem here...?
> 
> Sorry, what I meant was:
> 
>   1. Setting sa_mask is only meaningful if you set a signal handler; I don't
> think setting it is meaningful here because you're using SIG_DFL.
>   2. Even if we were registering a handler here, then because we're not
setting
> SA_NODEFER, SIGSEGV would be masked anyway.
> 
> So I think this sigaddset is unnecessary (although not harmful either).

Oh, I misunderstood, and now fixed. Thanks!

[hidehiko, 2015-04-17 06:18:47]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2015-04-17 06:18:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdempsky@chromium.org
Link to the patchset: https://codereview.chromium.org/1077143002/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-04-17 06:19:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1077143002/160001

[commit-bot: I haz the power, 2015-04-17 08:47:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-17 08:47:34]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hidehiko, 2015-04-17 13:47:03]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2015-04-17 13:47:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1077143002/160001

[commit-bot: I haz the power, 2015-04-17 14:59:41]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2015-04-17 15:00:34]

Patchset 5 (id:??) landed as
https://crrev.com/3a315e3246a5191cee27216c09e669ea2d6c33d3
Cr-Commit-Position: refs/heads/master@{#325634}

[johnme, 2015-04-17 16:02:31]

A revert of this CL (patchset #5 id:160001) has been created in
https://codereview.chromium.org/1093843002/ by johnme@chromium.org.

The reason for reverting is: Tentative revert to try to fix all the failing
tests on
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/b...
with stderrs like:

==22726== WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7fb677cf3d50 in sandbox::Trap::SigSys(int, siginfo*, void*)
sandbox/linux/seccomp-bpf/trap.cc:140:58
    #1 0x7fb6744e3fcf in sandbox::sys_sigaction(int, sigaction const*,
sigaction*) ???:0

  Uninitialized value was created by an allocation of 'temp.lvalue49' in the
stack frame of function '_ZN7sandbox7CodeGen6AppendEtjmm'
    #0 0x7fb677cf82b0 in sandbox::CodeGen::Append(unsigned short, unsigned int,
unsigned long, unsigned long) sandbox/linux/bpf_dsl/codegen.cc:121:0

SUMMARY: MemorySanitizer: use-of-uninitialized-value
(/b/build/slave/WebKit_Linux_MSAN/build/src/out/Release/content_shell+0xef94d50)
Exiting
[22595:22595:0417/081926:1015885843:ERROR:command_buffer_proxy_impl.cc(166)]
Could not send GpuCommandBufferMsg_Initialize.
[22595:22595:0417/081926:1015885969:ERROR:webgraphicscontext3d_command_buffer_impl.cc(209)]
CommandBufferProxy::Initialize failed.
[4:4:0417/081926:1015885973:ERROR:command_buffer_proxy_impl.cc(166)] Could not
send GpuCommandBufferMsg_Initialize.
[22595:22595:0417/081926:1015886031:ERROR:webgraphicscontext3d_command_buffer_impl.cc(226)]
Failed to initialize command buffer.
[4:4:0417/081926:1015886209:ERROR:webgraphicscontext3d_command_buffer_impl.cc(209)]
CommandBufferProxy::Initialize failed.
[4:4:0417/081926:1015886330:ERROR:webgraphicscontext3d_command_buffer_impl.cc(226)]
Failed to initialize command buffer..

[mdempsky, 2015-04-17 16:38:18]

On 2015/04/17 16:02:31, johnme wrote:
> ==22726== WARNING: MemorySanitizer: use-of-uninitialized-value
>     #0 0x7fb677cf3d50 in sandbox::Trap::SigSys(int, siginfo*, void*)
> sandbox/linux/seccomp-bpf/trap.cc:140:58
>     #1 0x7fb6744e3fcf in sandbox::sys_sigaction(int, sigaction const*,
> sigaction*) ???:0

I think this is a false positive relating to the fact that MemorySanitizer is
unaware the kernel is initializing that memory.

[Mark Seaborn, 2015-04-17 22:21:34]

On 17 April 2015 at 06:38, <mdempsky@chromium.org> wrote:

> On 2015/04/17 16:02:31, johnme wrote:
>
>> ==22726== WARNING: MemorySanitizer: use-of-uninitialized-value
>>      #0 0x7fb677cf3d50 in sandbox::Trap::SigSys(int, siginfo*, void*)
>> sandbox/linux/seccomp-bpf/trap.cc:140:58
>>      #1 0x7fb6744e3fcf in sandbox::sys_sigaction(int, sigaction const*,
>> sigaction*) ???:0
>>
>
> I think this is a false positive relating to the fact that MemorySanitizer
> is
> unaware the kernel is initializing that memory.


I think normally MSan wraps various libc syscall wrappers, such as
sigaction(), to annotate that the kernel is initialising memory.  If we
call syscalls directly, I suppose we have to supply those annotations
ourselves. :-/

Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mdempsky, 2015-04-17 22:24:51]

On Fri, Apr 17, 2015 at 3:21 PM, Mark Seaborn <mseaborn@chromium.org> wrote:

> I think normally MSan wraps various libc syscall wrappers, such as
> sigaction(), to annotate that the kernel is initialising memory.  If we
> call syscalls directly, I suppose we have to supply those annotations
> ourselves. :-/
>

That's what I suspect too. :(

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hidehiko, 2015-04-20 10:53:55]

On 2015/04/17 22:24:51, mdempsky wrote:
> On Fri, Apr 17, 2015 at 3:21 PM, Mark Seaborn <mailto:mseaborn@chromium.org>
wrote:
> 
> > I think normally MSan wraps various libc syscall wrappers, such as
> > sigaction(), to annotate that the kernel is initialising memory.  If we
> > call syscalls directly, I suppose we have to supply those annotations
> > ourselves. :-/
> >
> 
> That's what I suspect too. :(
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Hmm, this is unfortunate...

BTW, is there a way to run the test locally?
I've tried to run ./third_party/WebKit/Tools/Scripts/run-webkit-tests
--enable-sanitizer with the binary built with msan=1. However, the error is not
reported on my local env.