SECCOMP-BPF: Fix SandboxSyscall()

sandbox/linux/seccomp-bpf/syscall.h

Index: sandbox/linux/seccomp-bpf/syscall.h
diff --git a/sandbox/linux/seccomp-bpf/syscall.h b/sandbox/linux/seccomp-bpf/syscall.h
index 932e398015881d710759dd9295cc9fdabccb41fd..56771ca71bd94339edb6a6e16c99eab539c87e6b 100644
--- a/sandbox/linux/seccomp-bpf/syscall.h
+++ b/sandbox/linux/seccomp-bpf/syscall.h
@@ -5,7 +5,6 @@
 #ifndef SANDBOX_LINUX_SECCOMP_BPF_SYSCALL_H__
 #define SANDBOX_LINUX_SECCOMP_BPF_SYSCALL_H__
 
-#include <signal.h>
 #include <stdint.h>
 
 namespace playground2 {
@@ -16,7 +15,104 @@ namespace playground2 {
 // that also b) can be invoked in a way that computes this return address.
 // Passing "nr" as "-1" computes the "magic" return address. Passing any
 // other value invokes the appropriate system call.
-intptr_t SandboxSyscall(int nr, ...);
+intptr_t SandboxSyscall(int nr,
+                        intptr_t p0, intptr_t p1, intptr_t p2,

[Jeffrey Yasskin, 2012/12/03 22:27:35]

The way StrCat() and RE2 do this is to define a class like:

struct SyscallArg {
  SyscallArg(intptr_t i) : val(i) {}
  SyscallArg(const void* ptr) : val(reinterpret_cast<intptr_t>(ptr)) {}

  intptr_t val;
};

And then:
intptr_t SandboxSyscall(int nr,
    SyscallArg a0=SyscallArg(0), SyscallArg a1=SyscallArg(0),
    SyscallArg a2=SyscallArg(0), SyscallArg a3=SyscallArg(0),
    SyscallArg a4=SyscallArg(0), SyscallArg a5=SyscallArg(0));

This won't let you pass objects that have implicit conversions to intptr_t or
pointer types, but you couldn't generally pass those through ... anyway since
they're usually non-POD.

I don't have any problem with your current implementation; I'm just mentioning
this in case you prefer it.

[Markus (顧孟勤), 2012/12/04 00:54:39]

Yes, that would have worked too. I don't think it really buys us anything, and
it makes the code even more verbose and IMHO slightly harder to read. But I do
defer to your expertise, if for some reason you prefer this approach, I am happy
to switch to it.

On 2012/12/03 22:27:35, Jeffrey Yasskin wrote:

[Jeffrey Yasskin, 2012/12/04 01:18:48]

Nope, what you have is fine with me.

On 2012/12/04 00:54:39, Markus (顧孟勤) wrote:

+                        intptr_t p3, intptr_t p4, intptr_t p5);
+
+
+// System calls can take up to six parameters. Traditionally, glibc
+// implements this property by using variadic argument lists. This works, but
+// confuses modern tools such as valgrind, because we are nominally passing
+// uninitialized data whenever we call through this function and pass less
+// than the full six arguments.
+// So, instead, we use C++'s template system to achieve a very similar
+// effect. C++ automatically sets the unused parameters to zero for us, and
+// it also does the correct type expansion (e.g. from 32bit to 64bit) where
+// necessary.
+// We have to use C-style cast operators as we want to be able to accept both

[Jeffrey Yasskin, 2012/12/03 22:27:35]

You could use reinterpret_cast<intptr_t>(p0), etc. That nicely flags the fact
that you could be casting pointers here, although you've also commented that, so
it doesn't help a huge amount.

[Markus (顧孟勤), 2012/12/04 00:54:39]

I am not sure, I fully understand your comment.

I am concerned that using C++-style comments, I will either warn me because a) I
am converting a pointer to an integer type, or b) converting a 32bit value to a
64bit value. I am a little weak on understanding all the different cast
operators, but I don't think there is one that allows both operations.

Maybe, that's the reason why StrCat() went with the more complicated solution
that has different function prototypes.

On 2012/12/03 22:27:35, Jeffrey Yasskin wrote:

[Jeffrey Yasskin, 2012/12/04 01:18:48]

Ah, yes, reinterpret_cast can't convert between different integer types, so the
C-style cast is required.

On 2012/12/04 00:54:39, Markus (顧孟勤) wrote:

+// integer and pointer types.
+#if __cplusplus >= 201103  // C++11
+
+template<class T0 = intptr_t, class T1 = intptr_t, class T2 = intptr_t,
+         class T3 = intptr_t, class T4 = intptr_t, class T5 = intptr_t>
+inline intptr_t SandboxSyscall(int nr,
+                               T0 p0 = 0, T1 p1 = 0, T2 p2 = 0,
+                               T3 p3 = 0, T4 p4 = 0, T5 p5 = 0)
+  __attribute__((always_inline));

[Jeffrey Yasskin, 2012/12/03 22:27:35]

Is there a semantic reason this attribute is needed, or is it just for
performance? The compiler will almost always inline such short functions, so a
future maintainer will likely want to remove the attribute, so if it's needed
for correctness you should comment that, and if it's just for performance, you
should probably remove it.

[Markus (顧孟勤), 2012/12/04 00:54:39]

I'll add a comment. It is merely for the convenience of people looking at stack
traces in debug builds.

It seems, debug builds don't do the automatic inlining here, and it exposes
confusing implementation details in stack traces. I want to spare people from
trying to understand what is happening here, when they really couldn't care less
about the implementation of our system call API.

On 2012/12/03 22:27:35, Jeffrey Yasskin wrote:

+
+template<class T0, class T1, class T2, class T3, class T4, class T5>
+inline intptr_t SandboxSyscall(int nr,
+                               T0 p0, T1 p1, T2 p2, T3 p3, T4 p4, T5 p5) {
+  return SandboxSyscall(nr,
+                        (intptr_t)p0, (intptr_t)p1, (intptr_t)p2,
+                        (intptr_t)p3, (intptr_t)p4, (intptr_t)p5);
+}
+
+#else  // Pre-C++11
+
+// TODO(markus): C++11 has a much more concise and readable solution for
+//   expressing what we are doing here. Delete the fall-back code for older
+//   compilers as soon as we have fully switched to C++11
+
+template<class T0, class T1, class T2, class T3, class T4, class T5>
+inline intptr_t SandboxSyscall(int nr,
+                               T0 p0, T1 p1, T2 p2, T3 p3, T4 p4, T5 p5)
+  __attribute__((always_inline));
+template<class T0, class T1, class T2, class T3, class T4, class T5>
+inline intptr_t SandboxSyscall(int nr,
+                               T0 p0, T1 p1, T2 p2, T3 p3, T4 p4, T5 p5) {
+  return SandboxSyscall(nr,
+                        (intptr_t)p0, (intptr_t)p1, (intptr_t)p2,
+                        (intptr_t)p3, (intptr_t)p4, (intptr_t)p5);
+}
+
+template<class T0, class T1, class T2, class T3, class T4>
+inline intptr_t SandboxSyscall(int nr, T0 p0, T1 p1, T2 p2, T3 p3, T4 p4)
+  __attribute__((always_inline));
+template<class T0, class T1, class T2, class T3, class T4>
+inline intptr_t SandboxSyscall(int nr, T0 p0, T1 p1, T2 p2, T3 p3, T4 p4) {
+  return SandboxSyscall(nr, p0, p1, p2, p3, p4, 0);
+}
+
+template<class T0, class T1, class T2, class T3>
+inline intptr_t SandboxSyscall(int nr, T0 p0, T1 p1, T2 p2, T3 p3)
+  __attribute__((always_inline));
+template<class T0, class T1, class T2, class T3>
+inline intptr_t SandboxSyscall(int nr, T0 p0, T1 p1, T2 p2, T3 p3) {
+  return SandboxSyscall(nr, p0, p1, p2, p3, 0, 0);
+}
+
+template<class T0, class T1, class T2>
+inline intptr_t SandboxSyscall(int nr, T0 p0, T1 p1, T2 p2)
+  __attribute__((always_inline));
+template<class T0, class T1, class T2>
+inline intptr_t SandboxSyscall(int nr, T0 p0, T1 p1, T2 p2) {
+  return SandboxSyscall(nr, p0, p1, p2, 0, 0, 0);
+}
+
+template<class T0, class T1>
+inline intptr_t SandboxSyscall(int nr, T0 p0, T1 p1)
+  __attribute__((always_inline));
+template<class T0, class T1>
+inline intptr_t SandboxSyscall(int nr, T0 p0, T1 p1) {
+  return SandboxSyscall(nr, p0, p1, 0, 0, 0, 0);
+}
+
+template<class T0>
+inline intptr_t SandboxSyscall(int nr, T0 p0)
+  __attribute__((always_inline));
+template<class T0>
+inline intptr_t SandboxSyscall(int nr, T0 p0) {
+  return SandboxSyscall(nr, p0, 0, 0, 0, 0, 0);
+}
+
+inline intptr_t SandboxSyscall(int nr)
+  __attribute__((always_inline));
+inline intptr_t SandboxSyscall(int nr) {
+  return SandboxSyscall(nr, 0, 0, 0, 0, 0, 0);
+}
+
+#endif  // Pre-C++11
 
 }  // namespace