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 all 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
10 changes: 4 additions & 6 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 @@ -37,7 +32,10 @@ int main(int, char**)
assert(f.close() == nullptr);
assert(!f.is_open());
}
#if defined(__unix__)
// Starting with Android API 30+, Bionic's fdsan aborts a process that calls
// close() on a file descriptor tagged as belonging to something else (such
// as a FILE*).
#if defined(__unix__) && !defined(__BIONIC__)
{
std::filebuf f;
Copy link
Member

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

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

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

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

assert(!f.is_open());
Expand Down
Loading