Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 7, 2025
Merged

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

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
061580b
[libc++][Android] Disable fdsan in close.pass.cpp
rprichard Aug 8, 2024
285d243
formatting
rprichard Aug 8, 2024
9fe61c2
Merge branch 'main' into android-disable-fdsan
rprichard Dec 3, 2024
2825698
Skip part of the test for `__BIONIC__`
rprichard Dec 3, 2024
7f5419b
Merge branch 'main' into android-disable-fdsan
ldionne Mar 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions libcxx/test/std/input.output/file.streams/fstreams/filebuf.members/close.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__)
Expand All @@ -24,6 +19,15 @@
#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)
Copy link
Member

@DanAlbert DanAlbert Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@rprichard rprichard Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

__attribute__((weak));
#endif

int main(int, char**)
{
std::string temp = get_temp_file_name();
Expand All @@ -37,6 +41,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;
Copy link
Member

@ldionne ldionne Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@rprichard rprichard Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ldionne ldionne Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@rprichard rprichard Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down