Skip to content

Fix fdsan violation in StdoutLogger::Stop() on Android API 29+#17

Merged
CedricGuillemet merged 1 commit intoBabylonJS:mainfrom
CedricGuillemet:fix-fdsan-stdout-logger
Apr 21, 2026
Merged

Fix fdsan violation in StdoutLogger::Stop() on Android API 29+#17
CedricGuillemet merged 1 commit intoBabylonJS:mainfrom
CedricGuillemet:fix-fdsan-stdout-logger

Conversation

@CedricGuillemet
Copy link
Copy Markdown
Contributor

Problem

StdoutLogger::Start() calls fdopen(fd[0], "r") to hand each pipe read-end to a reader thread. fdopen() transfers ownership of the file descriptor to the FILE*; it must be released with fclose(), not close().

StdoutLogger::Stop() was calling close(fd_stdout[0]) and close(fd_stderr[0]) directly. On Android API 29+, fdsan tracks descriptor ownership and aborts with SIGABRT when a raw close() is called on a descriptor owned by a FILE*:

Fatal signal 6 (SIGABRT)
Abort message: 'fdsan: attempted to close file descriptor 94, expected to be unowned, actually owned by FILE* 0x6e357db180'
  #03 android::StdoutLogger::Stop()+184

Fix

In Stop(), only close the write ends (fd[1]). Closing fd[1] sends EOF through the pipe; getline() in the reader thread returns -1, exits the loop, and calls fclose(stream) — which correctly releases the read end. The explicit close(fd[0]) calls are removed entirely.

StdoutLogger::Start() calls fdopen(fd[0], "r") to create a FILE* for
each reader thread. Per POSIX, fdopen() transfers ownership of the file
descriptor to the FILE*; the caller must use fclose() — not close() —
to release it.

StdoutLogger::Stop() was calling close(fd_stdout[0]) and
close(fd_stderr[0]) directly. On Android API 29+, fdsan enforces
ownership tracking and aborts with SIGABRT when a raw close() is called
on a descriptor owned by a FILE*:

  Fatal signal 6 (SIGABRT)
  Abort message: 'fdsan: attempted to close file descriptor 94,
  expected to be unowned, actually owned by FILE* 0x6e357db180'

Fix: in Stop(), only close the write ends (fd[1]). Closing fd[1] sends
EOF through the pipe, causing getline() in the reader thread to return
-1, which breaks the loop and calls fclose(stream) — correctly closing
fd[0]. Remove the explicit close(fd[0]) / close(fd[1]) calls entirely.
Copilot AI review requested due to automatic review settings April 20, 2026 16:03
CedricGuillemet added a commit to CedricGuillemet/JsRuntimeHost that referenced this pull request Apr 20, 2026
Replace the local PATCH_COMMAND workaround with a direct reference to
the fix commit on the fork (PR: BabylonJS/AndroidExtensions#17).
Remove patches/AndroidExtensions/StdoutLogger.cpp.

Once the PR is merged upstream, revert GIT_REPOSITORY back to
BabylonJS/AndroidExtensions and update GIT_TAG to the merge commit.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to prevent Android API 29+ fdsan aborts by avoiding close() on pipe read-ends that have been transferred to FILE* ownership via fdopen() in StdoutLogger.

Changes:

  • Updates StdoutLogger::Stop() to close only the pipe write-ends and stop directly closing pipe read-ends.
  • Adds explanatory comments about fdopen() ownership and fdsan behavior on Android API 29+.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Source/StdoutLogger.cpp
Comment thread Source/StdoutLogger.cpp
Comment thread Source/StdoutLogger.cpp
Copy link
Copy Markdown
Contributor

@bghgary bghgary left a comment

Choose a reason for hiding this comment

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

[Approved by Copilot on behalf of @bghgary]

LGTM.

A few pre-existing issues in this file, not introduced here and not blocking, but worth tracking as follow-ups:

  • Detached reader threads are never joined — Stop() returns while they may still be draining.
  • Original stdout/stderr fds aren't saved, so writes after Stop() hit a closed pipe (EPIPE).
  • pthread_create error check compares against -1, but it returns an errno code on failure; the error path also leaks the pipe fds. pipe()'s return value is ignored too.

@CedricGuillemet CedricGuillemet merged commit 5fe60c8 into BabylonJS:main Apr 21, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants