Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rtsan] Error while building rtsan_interceptors.cpp #100754

Closed
tuliom opened this issue Jul 26, 2024 · 12 comments · Fixed by #100986
Closed

[rtsan] Error while building rtsan_interceptors.cpp #100754

tuliom opened this issue Jul 26, 2024 · 12 comments · Fixed by #100986

Comments

@tuliom
Copy link
Contributor

tuliom commented Jul 26, 2024

We've been seeing this issue since 2024-07-23.

That makes me believe this started to happen after b177ac4 .

Error message:

FAILED: compiler-rt/lib/rtsan/CMakeFiles/RTRtsan.x86_64.dir/rtsan_interceptors.cpp.o 
/builddir/build/BUILD/llvm-19.0.0_pre20240723.g40954d7f9bb38b-build/llvm-project-40954d7f9bb38b2407fe48a524befc5216f13ccc/llvm/redhat-linux-build/./bin/clang++ --target=x86_64-redhat-linux-gnu -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/builddir/build/BUILD/llvm-19.0.0_pre20240723.g40954d7f9bb38b-build/llvm-project-40954d7f9bb38b2407fe48a524befc5216f13ccc/compiler-rt/lib/rtsan/.. -O2 -flto=thin -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS --config=/usr/lib/rpm/redhat/redhat-hardened-clang.cfg -fstack-protector-strong -m64 -march=x86-64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -D_DEFAULT_SOURCE -Dasm=__asm__ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -Wno-unused-parameter -O2 -g -DNDEBUG -std=c++17 -m64 -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -ftrivial-auto-var-init=pattern -DSANITIZER_COMMON_NO_REDEFINE_BUILTINS -MD -MT compiler-rt/lib/rtsan/CMakeFiles/RTRtsan.x86_64.dir/rtsan_interceptors.cpp.o -MF compiler-rt/lib/rtsan/CMakeFiles/RTRtsan.x86_64.dir/rtsan_interceptors.cpp.o.d -o compiler-rt/lib/rtsan/CMakeFiles/RTRtsan.x86_64.dir/rtsan_interceptors.cpp.o -c /builddir/build/BUILD/llvm-19.0.0_pre20240723.g40954d7f9bb38b-build/llvm-project-40954d7f9bb38b2407fe48a524befc5216f13ccc/compiler-rt/lib/rtsan/rtsan_interceptors.cpp
/builddir/build/BUILD/llvm-19.0.0_pre20240723.g40954d7f9bb38b-build/llvm-project-40954d7f9bb38b2407fe48a524befc5216f13ccc/compiler-rt/lib/rtsan/rtsan_interceptors.cpp:388:22: error: address of overloaded function 'openat' does not match required type 'unsigned long'
  388 |   INTERCEPT_FUNCTION(openat);
      |                      ^~~~~~
/builddir/build/BUILD/llvm-19.0.0_pre20240723.g40954d7f9bb38b-build/llvm-project-40954d7f9bb38b2407fe48a524befc5216f13ccc/compiler-rt/lib/rtsan/../interception/interception.h:371:71: note: expanded from macro 'INTERCEPT_FUNCTION'
  371 | # define INTERCEPT_FUNCTION(func) INTERCEPT_FUNCTION_LINUX_OR_FREEBSD(func)
      |                                                                       ^~~~
/builddir/build/BUILD/llvm-19.0.0_pre20240723.g40954d7f9bb38b-build/llvm-project-40954d7f9bb38b2407fe48a524befc5216f13ccc/compiler-rt/lib/rtsan/../interception/interception_linux.h:35:33: note: expanded from macro 'INTERCEPT_FUNCTION_LINUX_OR_FREEBSD'
   35 |       (::__interception::uptr)&(func),            \
      |                                 ^~~~
/builddir/build/BUILD/llvm-19.0.0_pre20240723.g40954d7f9bb38b-build/llvm-project-40954d7f9bb38b2407fe48a524befc5216f13ccc/compiler-rt/lib/rtsan/rtsan_interceptors.cpp:72:18: note: candidate function
   72 | INTERCEPTOR(int, openat, int fd, const char *path, int oflag, ...) {
      |                  ^
/usr/include/bits/fcntl2.h:195:1: note: candidate address cannot be taken because parameter 2 has pass_object_size attribute
  195 | openat (int __fd, __fortify_clang_overload_arg (const char *, ,__path),
      | ^
/usr/include/bits/fcntl2.h:185:1: note: candidate address cannot be taken because parameter 2 has pass_object_size attribute
  185 | openat (int __fd, __fortify_clang_overload_arg (const char *, ,__path),
      | ^
/usr/include/bits/fcntl2.h:181:1: note: candidate function
  181 | openat (int __fd, const char *__path, int __oflag, mode_t __mode, ...)
      | ^
1 error generated.

The full build log is available at: builder-live.log.gz

@tuliom
Copy link
Contributor Author

tuliom commented Jul 26, 2024

@davidtrevelyan , @cjappl , could you take a look at this, please?

@nikic
Copy link
Contributor

nikic commented Jul 26, 2024

Pretty sure that this failure is due to fortify source overloads in new glibc, not rtsan changes. (I left a message on slack about this yesterday.)

@davidtrevelyan
Copy link
Contributor

Thanks @tuliom and @nikic - let us know if there's anything we can do - happy to jump in to help if needed. We're both new to LLVM contribution, so will need support from the established LLVM team if there is any ambiguity as to where the fix for this should live :)

@cjappl
Copy link
Contributor

cjappl commented Jul 26, 2024

Yes, thanks for pinging. Do let us know if you need intervention or testing from us, we are happy to help.

@mgorny mgorny added this to the LLVM 19.X Release milestone Jul 27, 2024
@mgorny
Copy link
Member

mgorny commented Jul 27, 2024

This is also preventing 19.1.0-rc1 from building now.

@cjappl
Copy link
Contributor

cjappl commented Jul 27, 2024

Can someone point me in the right direction of how to build locally with fortified glibc? This is my first time encountering it.

For the 19.0 branch, I can recommend a band-aid fix. Because the full v1 of rtsan is unlikely to land in this version, we can simply jettison this interceptor for now and fix this problem before the v20 branch is cut.

I am away from my computer for the next couple days, but this patch should do it.

diff --git a/compiler-rt/lib/rtsan/rtsan_interceptors.cpp b/compiler-rt/lib/rtsan/rtsan_interceptors.cpp
index 833062c3de41..c0808a0d2510 100644
--- a/compiler-rt/lib/rtsan/rtsan_interceptors.cpp
+++ b/compiler-rt/lib/rtsan/rtsan_interceptors.cpp
@@ -71,20 +71,6 @@ INTERCEPTOR(int, open, const char *path, int oflag, ...) {
   return result;
 }
 
-INTERCEPTOR(int, openat, int fd, const char *path, int oflag, ...) {
-  // TODO Establish whether we should intercept here if the flag contains
-  // O_NONBLOCK
-  ExpectNotRealtime("openat");
-
-  va_list args;
-  va_start(args, oflag);
-  mode_t mode = va_arg(args, int);
-  va_end(args);
-
-  const int result = REAL(openat)(fd, path, oflag, mode);
-  return result;
-}
-
 INTERCEPTOR(int, creat, const char *path, mode_t mode) {
   // TODO Establish whether we should intercept here if the flag contains
   // O_NONBLOCK
@@ -387,7 +373,6 @@ void __rtsan::InitializeInterceptors() {
 #endif
 
   INTERCEPT_FUNCTION(open);
-  INTERCEPT_FUNCTION(openat);
   INTERCEPT_FUNCTION(close);
   INTERCEPT_FUNCTION(fopen);
   INTERCEPT_FUNCTION(fread);
diff --git a/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors.cpp b/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors.cpp
index f5b016089087..a5318a52e45c 100644
--- a/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors.cpp
+++ b/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors.cpp
@@ -178,12 +178,6 @@ TEST_F(RtsanFileTest, OpenDiesWhenRealtime) {
   ExpectNonRealtimeSurvival(func);
 }
 
-TEST_F(RtsanFileTest, OpenatDiesWhenRealtime) {
-  auto func = [this]() { openat(0, GetTemporaryFilePath(), O_RDONLY); };
-  ExpectRealtimeDeath(func, "openat");
-  ExpectNonRealtimeSurvival(func);
-}
-
 TEST_F(RtsanFileTest, OpenCreatesFileWithProperMode) {
   const int mode = S_IRGRP | S_IROTH | S_IRUSR | S_IWUSR;

Any advice anyone has in terms of how we can build or fix this more holistically would be great.

@mgorny
Copy link
Member

mgorny commented Jul 27, 2024

I'll test that and make a PR. Thanks!

That said, I'm wondering if it wouldn't be cleaner to revert all the RTSAN commits from 19.x. It looks like a last minute addition that really shouldn't have been done prior to the branching.

mgorny added a commit to mgorny/llvm-project that referenced this issue Jul 27, 2024
…ource builds

Remove the openat interceptor from the 19.x branch, as it currently
breaks builds against fortify-sources glibc, and full rtsan will not
be included in this version anyway.

Patch provided by Chris Apple in
llvm#100754 (comment)

Bug llvm#100754
@mgorny
Copy link
Member

mgorny commented Jul 27, 2024

#100876

@nikic
Copy link
Contributor

nikic commented Jul 27, 2024

Can someone point me in the right direction of how to build locally with fortified glibc? This is my first time encountering it.

Just adding -D_FORTIFY_SOURCE=2 to the cflags should be enough. The trickier bit is that you also need glibc 2.40.

Any advice anyone has in terms of how we can build or fix this more holistically would be great.

Adding an explicit cast to the expected function pointer type before casting to the intptr type should work, which avoids the overload ambiguity.


By the way, aren't these interceptors (for both open and openat) pretty broken? They seem to assume that the variadic mode argument is always passed, even though it will only be passed for some values of oflag.

@cjappl
Copy link
Contributor

cjappl commented Jul 27, 2024

I'll test that and make a PR. Thanks!

That said, I'm wondering if it wouldn't be cleaner to revert all the RTSAN commits from 19.x. It looks like a last minute addition that really shouldn't have been done prior to the branching.

I think that is completely fine as well! Whatever you think is best to unblock 19 :). I think it is likely all the pieces will be put together for 20, as the next major PRs are in flight

Again, thanks for the patience with us. We are new to the project and still learning all the dos/donts. Sincerely appreciate the guidance and help!

@cjappl
Copy link
Contributor

cjappl commented Jul 27, 2024

Can someone point me in the right direction of how to build locally with fortified glibc? This is my first time encountering it.

Just adding -D_FORTIFY_SOURCE=2 to the cflags should be enough. The trickier bit is that you also need glibc 2.40.

Any advice anyone has in terms of how we can build or fix this more holistically would be great.

Adding an explicit cast to the expected function pointer type before casting to the intptr type should work, which avoids the overload ambiguity.


By the way, aren't these interceptors (for both open and openat) pretty broken? They seem to assume that the variadic mode argument is always passed, even though it will only be passed for some values of oflag.

Thanks for this guidance, super helpful!

I'll go back and make sure the interceptors work as intended with all values. If we don't have unit tests for those cases (with va_args and without) I'll ensure the are added.

@nikic
Copy link
Contributor

nikic commented Jul 29, 2024

I've put up a possible fix at #100986.

@nikic nikic closed this as completed in 155b7a1 Jul 30, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Jul 30, 2024
Since glibc 2.40 some functions like openat make use of overloads when
built with `-D_FORTIFY_SOURCE=2`, see:
https://github.com/bminor/glibc/blob/master/io/bits/fcntl2.h

This means that doing something like `(uintptr_t) openat` or `(void *)
openat` is now ambiguous, breaking the compiler-rt build on new glibc
versions.

Fix this by explicitly casting the symbol to the expected function type
before casting it to an intptr. The expected type is obtained as
`decltype(REAL(func))` so we don't have to repeat the signature from
INTERCEPTOR in the INTERCEPT_FUNTION macro.

Fixes llvm#100754.

(cherry picked from commit 155b7a1)
tru pushed a commit to llvmbot/llvm-project that referenced this issue Aug 1, 2024
Since glibc 2.40 some functions like openat make use of overloads when
built with `-D_FORTIFY_SOURCE=2`, see:
https://github.com/bminor/glibc/blob/master/io/bits/fcntl2.h

This means that doing something like `(uintptr_t) openat` or `(void *)
openat` is now ambiguous, breaking the compiler-rt build on new glibc
versions.

Fix this by explicitly casting the symbol to the expected function type
before casting it to an intptr. The expected type is obtained as
`decltype(REAL(func))` so we don't have to repeat the signature from
INTERCEPTOR in the INTERCEPT_FUNTION macro.

Fixes llvm#100754.

(cherry picked from commit 155b7a1)
banach-space pushed a commit to banach-space/llvm-project that referenced this issue Aug 7, 2024
Since glibc 2.40 some functions like openat make use of overloads when
built with `-D_FORTIFY_SOURCE=2`, see:
https://github.com/bminor/glibc/blob/master/io/bits/fcntl2.h

This means that doing something like `(uintptr_t) openat` or `(void *)
openat` is now ambiguous, breaking the compiler-rt build on new glibc
versions.

Fix this by explicitly casting the symbol to the expected function type
before casting it to an intptr. The expected type is obtained as
`decltype(REAL(func))` so we don't have to repeat the signature from
INTERCEPTOR in the INTERCEPT_FUNTION macro.

Fixes llvm#100754.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

6 participants