[libc++][Android] Disable fdsan in filebuf close.pass.cpp #102412

rprichard · 2024-08-08T01:57:05Z

fdsan is Bionic's "File Descriptor Sanitizer". Starting in API 30+, it aborts this close.pass.cpp test, because it closes the FD belonging to std::filebuf's FILE*. For __BIONIC__, disable that part of the test.

fdsan is Bionic's "File Descriptor Sanitizer". Starting in API 30+, it aborts this close.pass.cpp test, because it closes the FD belonging to std::filebuf's FILE*. Use an fdsan API to suppress the diagnostic.

llvmbot · 2024-08-08T01:57:41Z

@llvm/pr-subscribers-libcxx

Author: Ryan Prichard (rprichard)

Changes

fdsan is Bionic's "File Descriptor Sanitizer". Starting in API 30+, it aborts this close.pass.cpp test, because it closes the FD belonging to std::filebuf's FILE*. Use an fdsan API to suppress the diagnostic.

Full diff: https://github.com/llvm/llvm-project/pull/102412.diff

1 Files Affected:

(modified) libcxx/test/std/input.output/file.streams/fstreams/filebuf.members/close.pass.cpp (+15-5)

diff --git a/libcxx/test/std/input.output/file.streams/fstreams/filebuf.members/close.pass.cpp b/libcxx/test/std/input.output/file.streams/fstreams/filebuf.members/close.pass.cpp
index e0338e6f619b7..fa2b3415b3cd3 100644
--- a/libcxx/test/std/input.output/file.streams/fstreams/filebuf.members/close.pass.cpp
+++ b/libcxx/test/std/input.output/file.streams/fstreams/filebuf.members/close.pass.cpp
@@ -10,11 +10,6 @@
 
 // basic_filebuf<charT,traits>* close();
 
-// This test closes an fd that belongs to a std::filebuf, and Bionic's fdsan
-// detects this and aborts the process, starting in Android R (API 30).
-// See D137129.
-// XFAIL: LIBCXX-ANDROID-FIXME && !android-device-api={{2[1-9]}}
-
 #include <fstream>
 #include <cassert>
 #if defined(__unix__)
@@ -24,6 +19,14 @@
 #include "test_macros.h"
 #include "platform_support.h"
 
+// If we're building for a lower __ANDROID_API__, the Bionic versioner will
+// omit the function declarations from fdsan.h. We might be running on a newer
+// API level, though, so declare the API function here using weak.
+#if defined(__BIONIC__)
+#include <android/fdsan.h>
+enum android_fdsan_error_level android_fdsan_set_error_level(enum android_fdsan_error_level new_level) __attribute__((weak));
+#endif
+
 int main(int, char**)
 {
     std::string temp = get_temp_file_name();
@@ -37,6 +40,13 @@ int main(int, char**)
         assert(f.close() == nullptr);
         assert(!f.is_open());
     }
+#if defined(__BIONIC__)
+    // Starting with Android API 30+, Bionic's fdsan aborts a process that
+    // attempts to close a file descriptor belonging to something else. Disable
+    // fdsan to allow closing the FD belonging to std::filebuf's FILE*.
+    if (android_fdsan_set_error_level != nullptr)
+        android_fdsan_set_error_level(ANDROID_FDSAN_ERROR_LEVEL_DISABLED);
+#endif
 #if defined(__unix__)
     {
         std::filebuf f;

github-actions · 2024-08-08T02:00:32Z

✅ With the latest revision this PR passed the C/C++ code formatter.

DanAlbert · 2024-08-08T23:22:58Z

libcxx/test/std/input.output/file.streams/fstreams/filebuf.members/close.pass.cpp

+// API level, though, so declare the API function here using weak.
+#if defined(__BIONIC__)
+#include <android/fdsan.h>
+enum android_fdsan_error_level android_fdsan_set_error_level(enum android_fdsan_error_level new_level)


Annoying that you can't use -D __ANDROID_UNAVAILABLE_SYMBOLS_ARE_WEAK__ for this one. If you want to match that behavior more closely, instead of making this weak, pass that anyway and then use __INTRODUCED_IN here? You'd also want -Werror=unguarded-availabilty. More or less the same behavior, but the callsite would then be __builtin_available like it would be if this feature worked properly in bionic, and means that some day when we do fix that the change to this file will just be deleting the duplicate prototype (if even that).

Not really a big deal, but it's an alternative if you prefer it.

Yeah, that seems like a good idea. I can try adding those two compile_flags arguments to llvm-libc++-android-ndk.cfg.in.

ldionne

I'm not a big fan of adding this complexity, at least not until I'm convinced that it's necessary. See my questions below.

ldionne · 2024-08-16T13:50:17Z

libcxx/test/std/input.output/file.streams/fstreams/filebuf.members/close.pass.cpp

+    // fdsan to allow closing the FD belonging to std::filebuf's FILE*.
+    if (android_fdsan_set_error_level != nullptr)
+        android_fdsan_set_error_level(ANDROID_FDSAN_ERROR_LEVEL_DISABLED);
+#endif
 #if defined(__unix__)
    {
        std::filebuf f;


Is what we're doing in this test Standards conforming? If so, is there a reason why fdsan flags it? That would seem like a false positive to me.

std::filebuf::__open isn't in the C++ standard, so that would make the the test non-conforming for C++.

Maybe there's a POSIX conformance question for the combination of libc++ and this test? Sometimes programs close the default input/output FDs (e.g. STDOUT_FILENO), even though those FDs are associated with FILE* (e.g. stdout). fdsan has special handling to accommodate the standard streams, https://android.googlesource.com/platform/bionic/+/refs/tags/android-14.0.0_r61/libc/stdio/stdio.cpp#110.

__open is using fdopen, and Bionic's fdopen uses android_fdsan_exchange_owner_tag here, https://android.googlesource.com/platform/bionic/+/refs/tags/android-14.0.0_r61/libc/stdio/stdio.cpp#237:

android_fdsan_exchange_owner_tag(fd, 0, __get_file_tag(fp));

This line asserts that the FD's existing tag is 0 (i.e. that the FD is unowned), and then marks the FD as owned by the new FILE*. Hence, the close() call in the libc++ test fails, because it doesn't provide the expected FD tag. I believe Bionic's behavior is intentional here.

This test is intentionally doing a double-close, which is the exact thing that fdsan was created to detect. https://android.googlesource.com/platform/bionic/+/master/docs/fdsan.md#background

For this test, I'd also be OK with adding !defined(__BIONIC__) to defined(__unix__), with a short comment about why Bionic is special.

Yes, I'd much rather simply skip this test on Bionic. Re-reading the test, it is indeed doing something wrong but it's guarding with #if defined(__unix__), which basically means "I know I'm doing something wrong, but defined(unix) lets me get away with it". Instead, we want to change that to "I know I'm doing something wrong, but defined(unix) && !defined(BIONIC) lets me get away with it", since Bionic doesn't let you get away with it.

Ok, I'm now skipping that part of the test using #if defined(__unix__) && !defined(__BIONIC__).

ldionne · 2024-11-28T21:03:57Z

libcxx/test/std/input.output/file.streams/fstreams/filebuf.members/close.pass.cpp

+    // fdsan to allow closing the FD belonging to std::filebuf's FILE*.
+    if (android_fdsan_set_error_level != nullptr)
+        android_fdsan_set_error_level(ANDROID_FDSAN_ERROR_LEVEL_DISABLED);
+#endif
 #if defined(__unix__)
    {
        std::filebuf f;


Yes, I'd much rather simply skip this test on Bionic. Re-reading the test, it is indeed doing something wrong but it's guarding with #if defined(__unix__), which basically means "I know I'm doing something wrong, but defined(unix) lets me get away with it". Instead, we want to change that to "I know I'm doing something wrong, but defined(unix) && !defined(BIONIC) lets me get away with it", since Bionic doesn't let you get away with it.

ldionne

LGTM but please make sure CI is green.

[libc++][Android] Disable fdsan in close.pass.cpp

061580b

fdsan is Bionic's "File Descriptor Sanitizer". Starting in API 30+, it aborts this close.pass.cpp test, because it closes the FD belonging to std::filebuf's FILE*. Use an fdsan API to suppress the diagnostic.

rprichard requested a review from DanAlbert August 8, 2024 01:57

rprichard requested a review from a team as a code owner August 8, 2024 01:57

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 8, 2024

rprichard changed the title ~~[libc++][Android] Disable fdsan in close.pass.cpp~~ [libc++][Android] Disable fdsan in filebuf close.pass.cpp Aug 8, 2024

formatting

285d243

DanAlbert approved these changes Aug 8, 2024

View reviewed changes

ldionne requested changes Aug 16, 2024

View reviewed changes

ldionne requested changes Nov 28, 2024

View reviewed changes

rprichard added 2 commits December 3, 2024 15:33

Merge branch 'main' into android-disable-fdsan

9fe61c2

Skip part of the test for __BIONIC__

2825698

ldionne approved these changes Mar 25, 2025

View reviewed changes

ldionne added the pending-ci Merging the PR is only pending completion of CI label Mar 25, 2025

Merge branch 'main' into android-disable-fdsan

7f5419b

ldionne merged commit 93aba1e into llvm:main May 7, 2025
86 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++][Android] Disable fdsan in filebuf close.pass.cpp #102412

[libc++][Android] Disable fdsan in filebuf close.pass.cpp #102412

rprichard commented Aug 8, 2024 •

edited

Loading

llvmbot commented Aug 8, 2024

github-actions bot commented Aug 8, 2024 •

edited

Loading

DanAlbert Aug 8, 2024

rprichard Aug 9, 2024

ldionne left a comment

ldionne Aug 16, 2024

rprichard Aug 20, 2024

ldionne Nov 28, 2024

rprichard Dec 11, 2024

ldionne Nov 28, 2024

ldionne left a comment

[libc++][Android] Disable fdsan in filebuf close.pass.cpp #102412

[libc++][Android] Disable fdsan in filebuf close.pass.cpp #102412

Conversation

rprichard commented Aug 8, 2024 • edited Loading

llvmbot commented Aug 8, 2024

github-actions bot commented Aug 8, 2024 • edited Loading

DanAlbert Aug 8, 2024

Choose a reason for hiding this comment

rprichard Aug 9, 2024

Choose a reason for hiding this comment

ldionne left a comment

Choose a reason for hiding this comment

ldionne Aug 16, 2024

Choose a reason for hiding this comment

rprichard Aug 20, 2024

Choose a reason for hiding this comment

ldionne Nov 28, 2024

Choose a reason for hiding this comment

rprichard Dec 11, 2024

Choose a reason for hiding this comment

ldionne Nov 28, 2024

Choose a reason for hiding this comment

ldionne left a comment

Choose a reason for hiding this comment

rprichard commented Aug 8, 2024 •

edited

Loading

github-actions bot commented Aug 8, 2024 •

edited

Loading