This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Use canonical syscalls everywhere
ClosedPublic

Authored by eugenis on Apr 21 2022, 4:17 PM.

Details

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.

Diff Detail

Event Timeline

eugenis created this revision.Apr 21 2022, 4:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2022, 4:17 PM
eugenis requested review of this revision.Apr 21 2022, 4:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2022, 4:17 PM
Herald added subscribers: Restricted Project, pcwang-thead. · View Herald Transcript
vitalybuka accepted this revision.Apr 21 2022, 4:51 PM
vitalybuka added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
346
This revision is now accepted and ready to land.Apr 21 2022, 4:51 PM
eugenis updated this revision to Diff 424329.Apr 21 2022, 4:54 PM

address comments

pcc added inline comments.Apr 21 2022, 4:56 PM
compiler-rt/lib/sanitizer_common/sanitizer_platform.h
300–301

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

eugenis updated this revision to Diff 424333.Apr 21 2022, 5:08 PM

address comments

compiler-rt/lib/sanitizer_common/sanitizer_platform.h
300–301

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

pcc added inline comments.Apr 21 2022, 5:20 PM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
356–365

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

389–398

Likewise.

eugenis marked an inline comment as done.Apr 21 2022, 5:46 PM
eugenis added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
356–365

I think it is! Removed.

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

pcc accepted this revision.Apr 22 2022, 11:14 AM

LGTM

This revision was automatically updated to reflect the committed changes.

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 added a subscriber: thakis.Apr 28 2022, 6:26 AM

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.

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.

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.

vitalybuka reopened this revision.Apr 28 2022, 4:19 PM
This revision is now accepted and ready to land.Apr 28 2022, 4:19 PM
eugenis planned changes to this revision.Apr 28 2022, 5:28 PM

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

eugenis updated this revision to Diff 426179.Apr 29 2022, 3:25 PM

Fix internal_fork on s390.

This revision is now accepted and ready to land.Apr 29 2022, 3:25 PM

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?)

Thanks!

(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.

This revision was automatically updated to reflect the committed changes.

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 added a subscriber: jrtc27.May 12 2022, 11:54 AM

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

glaubitz added a comment.EditedMay 12 2022, 12:24 PM

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 ;).

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.

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.

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