ucontext_t support for Android x86.

sandbox/linux/services/ucontext_unittest.cc

Index: sandbox/linux/services/ucontext_unittest.cc
diff --git a/sandbox/linux/services/ucontext_unittest.cc b/sandbox/linux/services/ucontext_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..159a0eb739692d493fc492cda9fcceff4bc86925
--- /dev/null
+++ b/sandbox/linux/services/ucontext_unittest.cc
@@ -0,0 +1,98 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <setjmp.h>
+#include <signal.h>
+#include "sandbox/linux/seccomp-bpf/sandbox_bpf.h"
+
+#if defined(__arm__)
+#include "sandbox/linux/services/android_arm_ucontext.h"
+#elif defined(__i386__)
+#include "sandbox/linux/services/android_x86_ucontext.h"
+#else

[Markus (顧孟勤), 2013/01/08 22:30:17]

This is a useful unittest even if we are building for Linux as opposed to
Android.

We should make sure it is used on Linux and then, of course, we have to make
sure it runs on both i386 and x86-64, as well as ARM.

+#error "Unsupport CPU ABI"
+#endif
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {

[jln (very slow on Chromium), 2013/01/08 22:26:09]

You're in sandbox.

Please look at any other _unittest.c file in Sandbox.

+namespace android {

[Markus (顧孟勤), 2013/01/08 22:30:17]

This should not be Android specific code. Just follow the examples in the other
unittests that we already have.

+
+typedef testing::Test ucontext_test;

[Markus (顧孟勤), 2013/01/08 22:30:17]

This sounds wrong, as far as our style guide is concerned. But Julien is that
style-guide-god -- he probably can tell you better.

+
+sigjmp_buf mark;
+
+static int par1_v = 0xeb;
+static int par2_v = 0xec;
+static int par3_v = 0xed;
+static int par4_v = 0xee;
+static int par5_v = 0x00;

[Markus (顧孟勤), 2013/01/08 22:30:17]

Our style guide prefers the use of anonymous namespaces instead of "static".

Also, these should be named something like kExpectedValueParm1, or some other
similarly appropriate name.

And they should be marked as "const".

+
+void sig_action(int n, siginfo_t *siginfo, void* context)

[Markus (顧孟勤), 2013/01/08 22:30:17]

While I personally think our style guide is mistaken in where it wants the "*",
at the very least make sure you are consistent in your choice of where you put
it.

The style guide wants a different capitalization for this function name.

Also, this function should be in an anonymous namespace.

+{

[Markus (顧孟勤), 2013/01/08 22:30:17]

The brace should probably be at the end of the previous line.

+  ucontext_t *ctx = (ucontext_t *)context;

[Markus (顧孟勤), 2013/01/08 22:30:17]

According to our style guide, we should be using C++ style casts, rather than C
casts.

Maybe, rename "context" to "void_context"; and "ctx" to "context"? That makes it
a little more clear what is happening here.

[yfw.chromium, 2013/01/09 02:38:14]

Yes. several C vs C++. :).

+  sigset_t set = 0;
+
+  sigaddset(&set, SIGPIPE);
+  sigaddset(&set, SIGFPE);

[Markus (顧孟勤), 2013/01/08 22:30:17]

This is a little fragile. Both glibc and the kernel like to intercept
sigprocmask() and make some of the signal bits immutable.

The code would be more reliable, if you queried the current signal mask prior to
raising the signal. Then store it in a global variable that you can check from
within the signal handler.

Or if you don't want to use a global variable, store a pointer to it in a CPU
register. After all, you are looking at CPU registers already.

[yfw.chromium, 2013/01/09 02:38:14]

No. If kernel changes the signal mask between we store it and signal handle is
called. The thing is still broken. Frankly, I don't think kernel could play with
signal mask a lot. Not sure about glibc.

+
+  /* ARM define registers array as unsigned while x86 define it as signed */
+  EXPECT_EQ((unsigned long int)SECCOMP_PARM1(ctx), (unsigned long int)par1_v);
+  EXPECT_EQ((unsigned long int)SECCOMP_PARM2(ctx), (unsigned long int)par2_v);
+  EXPECT_EQ((unsigned long int)SECCOMP_PARM3(ctx), (unsigned long int)par3_v);
+  EXPECT_EQ((unsigned long int)SECCOMP_PARM4(ctx), (unsigned long int)par4_v);
+  EXPECT_EQ((unsigned long int)SECCOMP_PARM5(ctx), (unsigned long int)par5_v);
+  EXPECT_EQ(ctx->uc_sigmask, set);

[Markus (顧孟勤), 2013/01/08 22:30:17]

Use the comparison macros that we wrote for the sandbox. They can safely be
called from within a sub-function or a signal handler.

The EXPECT_XXX() macros OTOH must not be called from a signal handler.

+  siglongjmp(mark, -1);
+}
+
+TEST_F(ucontext_test, TestUcontext) {

[jln (very slow on Chromium), 2013/01/08 22:26:09]

Please use a SANDBOX_TEST so that you get to run in your own process etc.
Otherwise other tests will share the side effects of this.

[Markus (顧孟勤), 2013/01/08 22:30:17]

Use the test macros that we wrote for the sandbox. They do an extra fork(). This
allows you to make global state changes (such as messing with signal handlers),
without breaking other tests.

[yfw.chromium, 2013/01/09 02:38:14]

So I suppose that it's ok child process receiving signal like SIGSEGV and we
could remove sigsetjmp/siglongjmp. That's good.

+  int ret;
+  struct sigaction act;

[Markus (顧孟勤), 2013/01/08 22:30:17]

If you write "struct sigaction act = { }", you don't need the memset()

+  struct sigaction oact;
+
+  sigset_t new_set, old_set;
+
+  memset(&act, 0, sizeof(act));
+  act.sa_sigaction = sig_action;
+  act.sa_flags = SA_RESTART | SA_SIGINFO;

[Markus (顧孟勤), 2013/01/08 22:30:17]

Why do you set SA_RESTART? I don't think that even makes sense on SIGSEGV -- or
am I missing something.

[yfw.chromium, 2013/01/09 02:38:14]

I tried to minimize the impact of the test (Before I is told to use sandbox
unittest).

+  sigemptyset(&act.sa_mask);
+
+  sigemptyset(&new_set);
+  sigaddset(&new_set, SIGPIPE);
+  sigaddset(&new_set, SIGFPE);
+
+  sigprocmask(SIG_SETMASK, &new_set, &old_set);
+
+  if (sigsetjmp(mark, 1) != -1) {
+    ret = sigaction(SIGSEGV, &act, &oact);
+    EXPECT_EQ(ret, 0);
+
+#if defined(__i386__)

[Markus (顧孟勤), 2013/01/08 22:30:17]

I really would prefer if you tried to avoid using assembly. It just makes it
easier for other people to modify the code.

But if you absolutely have to use assembly, you have to make sure you set the
appropriate constraints to inform the compiler that you just clobbered a whole
bunch of registers.

Try:

register int ebx asm("ebx") = kExpectedValueParm1;

Not all of the registers will be available to you, as the compiler needs some
for internal uses.

Make sure you compile with -fPIC and with frame pointers in order to make sure
your code is compatible with these flags.

In order to raise a signal, you can do:

*(volatile char *)0 = 0; // Trigger a SIGSEGV

[yfw.chromium, 2013/01/09 02:38:14]

No. toolchain could use registers for
*(volatile char *)0 = 0;
and it heavily depends on the toolchain. Which make hard to select which
register should be checked. That's why I use assembly here.

register int ebx asm("ebx") = kExpectedValueParm1; doesn't work either because
the registers could be reused by toolchain again. I checked it on android
x86/ARM.

Clobbering registers is not big deal in this special case because the signal
handler will restore the saved context which include original value for
registers.

[yfw.chromium, 2013/01/09 02:38:14]

Done.

+    asm __volatile__ (
+      "movl $0xeb, %ebx\n\t"
+      "movl $0xec, %ecx\n\t"
+      "movl $0xed, %edx\n\t"
+      "movl $0xee, %esi\n\t"
+      "movl $0x00, %edi\n\t"
+      "movl $0x00, (%edi)\n\t"
+      );
+#elif defined(__arm__)
+    asm __volatile__ (
+      "mov r0, #0xeb\n\t"
+      "mov r1, #0xec\n\t"
+      "mov r2, #0xed\n\t"
+      "mov r3, #0xee\n\t"
+      "mov r4, #0x00\n\t"
+      "str r3, [r4]\n\t"
+    );
+#endif
+  }

[Markus (顧孟勤), 2013/01/08 22:30:17]

How about you change one or more of the CPU registers from within the signal
handler. Then you can verify that the change took place and you also verify that
the signal handler ran.

Although, come to think of it ... this might not work with sigsetjmp() :-(
That's really too bad. Maybe we should try to get rid of the sigsetjmp(); not
sure how to do that cleanly, though.

[yfw.chromium, 2013/01/09 02:38:14]

If want to make sure the signal handler is called, use a global flag?

+
+  sigprocmask(SIG_SETMASK, &old_set, NULL);
+  sigaction(SIGSEGV, &oact, NULL);
+}
+
+}  // namespace android
+}  // namespace base