[sanitizer] Use canonical syscalls everywhere

[eugenis]

Summary

These "new" syscalls have been added in 2.6.16, more than 16 years ago.
Surely that's enough time to migrate. Glibc 2.33 is using them on both
i386 and x86_64. Android has an selinux filter to block the legacy
syscalls in the apps.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp

346

[eugenis]

address comments

[pcc]

compiler-rt/lib/sanitizer_common/sanitizer_platform.h

305

Can we remove this macro and replace it with SANITIZER_LINUX everywhere?

[eugenis]

address comments

compiler-rt/lib/sanitizer_common/sanitizer_platform.h

305

Sure. I wanted to preserve some context in the source, but that does not really matter.

[pcc]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp

364–373

Is this code dead now? I think SANITIZER_LINUX_USES_64BIT_SYSCALLS can only be true if SANITIZER_LINUX is.

397–406

Likewise.

[eugenis]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp

364–373

I think it is! Removed.

We also need to do something prepocessor code formatting. This spacing is insane and makes the code unreadable.

[eugenis]

.

[pcc]

LGTM

[uweigand]

This seems to have broken the s390x build bots, there are now >500 failing sanitizer test cases:
https://lab.llvm.org/buildbot/#/builders/94/builds/8761

Not sure yet what exactly the problem is; those syscalls certainly should be available, but we may run into some other issues in these new code paths.

[thakis]

Reverting this didn't help for our Android issue. I'll leave relanding to you because of the s390x comment further up, but from our end I think this is good for relanding.

[eugenis]

Thanks. Ulrich, could you help me with the s390 failures? I can reland with a #define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS !SANITIZER_S390 for now, but it would be great to remove the non-canonical code path completely.

[uweigand]

In D124212#3480335, @eugenis wrote:

Thanks. Ulrich, could you help me with the s390 failures? I can reland with a #define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS !SANITIZER_S390 for now, but it would be great to remove the non-canonical code path completely.

Looks like the problem is in internal_fork. This code path:

return internal_syscall(SYSCALL(clone), SIGCHLD, 0);

fails on s390x because the low-level clone syscall interface is different between architectures. To quote the Linux manpage:

       The raw system call interface on x86-64 and some other architectures (including sh, tile, and alpha) is roughly:

           long clone(unsigned long flags, void *child_stack,
                      int *ptid, int *ctid,
                      unsigned long newtls);
[... ]
       On the cris and s390 architectures, the order of the first two arguments is reversed:

           long clone(void *child_stack, unsigned long flags,
                      int *ptid, int *ctid,
                      unsigned long newtls);

This means that instead of passing NULL as child stack pointer, we're passing SIGCHLD (0x11), and so the child crashes when accessing the stack.

[eugenis]

Thanks! I'll update the patch later. Apparently there is like 5 different signatures...

[eugenis]

Fix internal_fork on s390.

[uweigand]

LGTM for SystemZ.

(As an aside, what is the reason for not just using the fork syscall anyway? I thought fork should be completely equivalent to clone with the termination signal set to SIGCHLD and no other flags?)

[eugenis]

Thanks!

In D124212#3485433, @uweigand wrote:

(As an aside, what is the reason for not just using the fork syscall anyway? I thought fork should be completely equivalent to clone with the termination signal set to SIGCHLD and no other flags?)

Primarily, Android's seccomp sandbox only allows fork as an exception for 32-bits apps but not for the system binaries. Arm64 does not even have a fork syscall; glibc uses clone on all targets.

This feels like moving in the right direction.

[glaubitz]

This change unfortunately broke the build on Linux/sparc64.

I'm not sure whether newfstatat is supposed to be available on Linux/sparc64 [1]:

FAILED: projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonNoHooks.sparcv9.dir/sanitizer_linux.cpp.o 
/usr/bin/c++ -DHAVE_RPC_XDR_H=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/stage1/projects/compiler-rt/lib/sanitizer_common -I/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common -I/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/stage1/include -I/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/llvm/include -I/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/.. -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -std=c++14 -Wno-unused-parameter -O3 -DNDEBUG  -m64 -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fvisibility=hidden -fno-lto -O3 -g -Wno-variadic-macros -nostdinc++ -Wno-format -fno-rtti -Wframe-larger-than=570 -DSANITIZER_SUPPORTS_WEAK_HOOKS=0 -UNDEBUG -std=c++14 -MD -MT projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonNoHooks.sparcv9.dir/sanitizer_linux.cpp.o -MF projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonNoHooks.sparcv9.dir/sanitizer_linux.cpp.o.d -o projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonNoHooks.sparcv9.dir/sanitizer_linux.cpp.o -c /var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
In file included from /var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:191:
/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp: In function ‘__sanitizer::uptr __sanitizer::internal_stat(const char*, void*)’:
/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_syscall_generic.inc:19:24: error: ‘__NR_newfstatat’ was not declared in this scope
   19 | # define SYSCALL(name) __NR_ ## name
      |                        ^~~~~
/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:347:27: note: in expansion of macro ‘SYSCALL’
  347 |   return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path, (uptr)buf,
      |                           ^~~~~~~
/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp: In function ‘__sanitizer::uptr __sanitizer::internal_lstat(const char*, void*)’:
/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_syscall_generic.inc:19:24: error: ‘__NR_newfstatat’ was not declared in this scope
   19 | # define SYSCALL(name) __NR_ ## name
      |                        ^~~~~
/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:370:27: note: in expansion of macro ‘SYSCALL’
  370 |   return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path, (uptr)buf,
      |                           ^~~~~~~
/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp: In member function ‘__sanitizer::SignalContext::WriteFlag __sanitizer::SignalContext::GetWriteFlag() const’:
/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:1818:12: warning: unused variable ‘ucontext’ [-Wunused-variable]
 1818 |   Context *ucontext = (Context *)context;
      |            ^~~~~~~~

Any idea how to fix this? Do we just need to wire up newfstatat in the kernel on SPARC?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/sparc/kernel/syscalls/syscall.tbl

[jrtc27]

There's a lot of really quite wonky whitespace reformatting in this diff

[glaubitz]

In D124212#3509781, @jrtc27 wrote:

There's a lot of really quite wonky whitespace reformatting in this diff

FWIW, I have already a draft for fixing this. Will create a revision tomorrow.

EDIT: I mean the build failure on sparc64 ;).

[eugenis]

In D124212#3509846, @glaubitz wrote:

In D124212#3509781, @jrtc27 wrote:

There's a lot of really quite wonky whitespace reformatting in this diff

FWIW, I have already a draft for fixing this. Will create a revision tomorrow.

EDIT: I mean the build failure on sparc64 ;).

Thanks. Sorry for the breakage. It appears that sparc64 is the only 64-bit target without newfstatat syscall.

I've considered keeping the non-canonical-syscalls code path for cases like this, but getting rid of all the preprocessor conditionals was too tempting :)

We should re-lint this entire file at some point, but there is never a good time for that - you don't want to mix it with a functional change, and then it also makes it harder to revert changes so you'd want to wait a while after any non-trivial commit... Maybe after you land the sparc fix.

[jrtc27]

In D124212#3509972, @eugenis wrote:

In D124212#3509846, @glaubitz wrote:

In D124212#3509781, @jrtc27 wrote:

There's a lot of really quite wonky whitespace reformatting in this diff

FWIW, I have already a draft for fixing this. Will create a revision tomorrow.

EDIT: I mean the build failure on sparc64 ;).

Thanks. Sorry for the breakage. It appears that sparc64 is the only 64-bit target without newfstatat syscall.

That's not true, alpha and hppa64 are the same from what I can tell, maybe others too, I stopped looking after that point, but sparc64 may be the only architecture compiler-rt supports that's like that.

I've considered keeping the non-canonical-syscalls code path for cases like this, but getting rid of all the preprocessor conditionals was too tempting :)

We should re-lint this entire file at some point, but there is never a good time for that - you don't want to mix it with a functional change, and then it also makes it harder to revert changes so you'd want to wait a while after any non-trivial commit... Maybe after you land the sparc fix.

Sure, but you should never be changing the whitespace for #else without also changing #if to match (or vice-versa, or ditto with #endif), currently you have mismatched indentation all over the place which is what I meant by wonky, it's extremely misleading and hard to read.

[glaubitz]

Maybe after you land the sparc fix.

In case you have time for review, I have posted the SPARC fix: https://reviews.llvm.org/D125572

compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp

 #endif
 }
 #endif
 
 uptr internal_stat(const char *path, void *buf) {
 #if SANITIZER_FREEBSD
   return internal_syscall(SYSCALL(fstatat), AT_FDCWD, (uptr)path, (uptr)buf, 0);
 #elif SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
+#      if SANITIZER_WORDSIZE == 64
   return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path, (uptr)buf,
                           0);
+#      else
+  struct stat64 buf64;
+  int res = internal_syscall(SYSCALL(fstatat64), AT_FDCWD, (uptr)path,
+                             (uptr)&buf64, 0);
+  stat64_to_stat(&buf64, (struct stat *)buf);
+  return res;
+#      endif
 #elif SANITIZER_LINUX_USES_64BIT_SYSCALLS
 # if defined(__mips64)
   // For mips64, stat syscall fills buffer in the format of kernel_stat
   struct kernel_stat kbuf;
   int res = internal_syscall(SYSCALL(stat), path, &kbuf);
   kernel_stat_to_stat(&kbuf, (struct stat *)buf);
   return res;
 # else
   return internal_syscall(SYSCALL(stat), (uptr)path, (uptr)buf);
 # endif
 #else
   struct stat64 buf64;
   int res = internal_syscall(SYSCALL(stat64), path, &buf64);
   stat64_to_stat(&buf64, (struct stat *)buf);
   return res;
 #endif
 }
 
 uptr internal_lstat(const char *path, void *buf) {
 #if SANITIZER_FREEBSD
   return internal_syscall(SYSCALL(fstatat), AT_FDCWD, (uptr)path, (uptr)buf,
                           AT_SYMLINK_NOFOLLOW);
 #elif SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
+#      if defined(_LP64)
   return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path, (uptr)buf,
                           AT_SYMLINK_NOFOLLOW);
+#      else
+  struct stat64 buf64;
+  int res = internal_syscall(SYSCALL(fstatat64), AT_FDCWD, (uptr)path,
+                             (uptr)&buf64, AT_SYMLINK_NOFOLLOW);
+  stat64_to_stat(&buf64, (struct stat *)buf);
+  return res;
+#      endif
 #elif SANITIZER_LINUX_USES_64BIT_SYSCALLS
 # if SANITIZER_MIPS64
   // For mips64, lstat syscall fills buffer in the format of kernel_stat
   struct kernel_stat kbuf;
   int res = internal_syscall(SYSCALL(lstat), path, &kbuf);
   kernel_stat_to_stat(&kbuf, (struct stat *)buf);
   return res;
 # else
   return internal_syscall(SYSCALL(lstat), (uptr)path, (uptr)buf);
 # endif
 #else
   struct stat64 buf64;
   int res = internal_syscall(SYSCALL(lstat64), path, &buf64);
   stat64_to_stat(&buf64, (struct stat *)buf);
   return res;
 #endif
 }
 
 uptr internal_fstat(fd_t fd, void *buf) {
 #if SANITIZER_FREEBSD || SANITIZER_LINUX_USES_64BIT_SYSCALLS
 #if SANITIZER_MIPS64
   // For mips64, fstat syscall fills buffer in the format of kernel_stat
   struct kernel_stat kbuf;
   int res = internal_syscall(SYSCALL(fstat), fd, &kbuf);
   kernel_stat_to_stat(&kbuf, (struct stat *)buf);
   return res;

 }
 #endif  // !SANITIZER_NETBSD
 
 // ----------------- sanitizer_common.h
 bool FileExists(const char *filename) {
   if (ShouldMockFailureToOpen(filename))
     return false;
   struct stat st;
-#if SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
-  if (internal_syscall(SYSCALL(newfstatat), AT_FDCWD, filename, &st, 0))
-#else
   if (internal_stat(filename, &st))
-#endif
     return false;
   // Sanity check: filename is a regular file.
   return S_ISREG(st.st_mode);
 }
 
 bool DirExists(const char *path) {
   struct stat st;
-#  if SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
-  if (internal_syscall(SYSCALL(newfstatat), AT_FDCWD, path, &st, 0))
-#  else
   if (internal_stat(path, &st))
-#  endif
     return false;
   return S_ISDIR(st.st_mode);
 }
 
 #  if !SANITIZER_NETBSD
 tid_t GetTid() {
 #if SANITIZER_FREEBSD
   long Tid;

 // ----------------- sanitizer_linux.h
 // The actual size of this structure is specified by d_reclen.
 // Note that getdents64 uses a different structure format. We only provide the
 // 32-bit syscall here.
 #if SANITIZER_NETBSD
 // Not used
 #else
 struct linux_dirent {
-#if SANITIZER_X32 || defined(__aarch64__) || SANITIZER_RISCV64
+#    if SANITIZER_X32 || SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
   u64 d_ino;
   u64 d_off;
-#else
+#    else
   unsigned long      d_ino;
   unsigned long      d_off;
-#endif
+#    endif
   unsigned short     d_reclen;
-#if defined(__aarch64__) || SANITIZER_RISCV64
+#    if SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
   unsigned char      d_type;
-#endif
+#    endif
   char               d_name[256];
 };
 #endif
 
 #if !SANITIZER_SOLARIS && !SANITIZER_NETBSD
 // Syscall wrappers.
 uptr internal_ptrace(int request, int pid, void *addr, void *data) {
   return internal_syscall(SYSCALL(ptrace), request, pid, (uptr)addr,

compiler-rt/lib/sanitizer_common/sanitizer_platform.h

 #else
 #  define SANITIZER_SIGN_EXTENDED_ADDRESSES 0
 #endif
 
 // The AArch64 and RISC-V linux ports use the canonical syscall set as
 // mandated by the upstream linux community for all new ports. Other ports
 // may still use legacy syscalls.
 #ifndef SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
-#  if (defined(__aarch64__) || defined(__riscv) || defined(__hexagon__)) && \
-      SANITIZER_LINUX
-#    define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS 1
-#  else
-#    define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS 0
-#  endif
+#  define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS SANITIZER_LINUX
 #endif
 
 // udi16 syscalls can only be used when the following conditions are
 // met:
 // * target is one of arm32, x86-32, sparc32, sh or m68k
 // * libc version is libc5, glibc-2.0, glibc-2.1 or glibc-2.2 to 2.15
 //   built against > linux-2.2 kernel headers
 // Since we don't want to include libc headers here, we check the