[sanitizer_common] child_stdin_fd_ should only be set on posix_spawn path #171508
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Andrew Haberlandt (ndrewh) Changes#170809 added the child_stdin_fd_ field on SymbolizerProcess to allow the parent process to hold on to the read in of the child's stdin pipe. This was to avoid SIGPIPE. However, the llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp Lines 525 to 535 in 7f5ed91 This could cause a double-close of this fd (problematic in the case of fd reuse). This moves the Full diff: https://github.com/llvm/llvm-project/pull/171508.diff 1 Files Affected:
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp
index 29c73e3e1cac1..ab6aee7c9fba7 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cpp
@@ -176,6 +176,10 @@ bool SymbolizerProcess::StartSymbolizerSubprocess() {
internal_close(outfd[1]);
return false;
}
+
+ // We intentionally hold on to the read-end so that we don't get a SIGPIPE
+ child_stdin_fd_ = outfd[0];
+
# else // SANITIZER_APPLE
UNIMPLEMENTED();
# endif // SANITIZER_APPLE
@@ -192,9 +196,6 @@ bool SymbolizerProcess::StartSymbolizerSubprocess() {
input_fd_ = infd[0];
output_fd_ = outfd[1];
- // We intentionally hold on to the read-end so that we don't get a SIGPIPE
- child_stdin_fd_ = outfd[0];
-
CHECK_GT(pid, 0);
// Check that symbolizer subprocess started successfully.
|
Contributor
Author
|
Auto-merging this to avoid any unintended impact to non-Darwin platforms caused by #170809 |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/206/builds/10287 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#170809 added the child_stdin_fd_ field on SymbolizerProcess to allow the parent process to hold on to the read in of the child's stdin pipe. This was to avoid SIGPIPE.
However, the
StartSubprocesspath still closes the stdin fd in the parent here:llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
Lines 525 to 535 in 7f5ed91
This could cause a double-close of this fd (problematic in the case of fd reuse).
This moves the
child_stdin_fd_field to only be initialized on the posix_spawn path. This should ensure #170809 only truly affects Darwin.