-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[Sanitizer] Option to fallback to stderr if unable to open logfile #158687
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.
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
Conversation
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Ellis Hoag (ellishg) ChangesAdd the santizier option Full diff: https://github.com/llvm/llvm-project/pull/158687.diff 4 Files Affected:
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_file.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
index 9236a458cdb0e..4e691d0768c52 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
@@ -36,9 +36,16 @@ void RawWrite(const char *buffer) {
void ReportFile::ReopenIfNecessary() {
mu->CheckLocked();
- if (fd == kStdoutFd || fd == kStderrFd) return;
+ if (!lastOpenFailed)
+ if (fd == kStdoutFd || fd == kStderrFd)
+ return;
uptr pid = internal_getpid();
+ if (lastOpenFailed && fd_pid != pid) {
+ fd = kInvalidFd;
+ lastOpenFailed = false;
+ }
+
// If in tracer, use the parent's file.
if (pid == stoptheworld_tracer_pid)
pid = stoptheworld_tracer_ppid;
@@ -48,8 +55,7 @@ void ReportFile::ReopenIfNecessary() {
// process, close it now.
if (fd_pid == pid)
return;
- else
- CloseFile(fd);
+ CloseFile(fd);
}
const char *exe_name = GetProcessName();
@@ -71,12 +77,17 @@ void ReportFile::ReopenIfNecessary() {
char errmsg[100];
internal_snprintf(errmsg, sizeof(errmsg), " (reason: %d)\n", err);
WriteToFile(kStderrFd, errmsg, internal_strlen(errmsg));
- Die();
+ if (!common_flags()->log_fallback_to_stderr)
+ Die();
+ const char *errmsg2 = "ERROR: Fallback to stderr\n";
+ WriteToFile(kStderrFd, errmsg2, internal_strlen(errmsg2));
+ lastOpenFailed = true;
+ fd = kStderrFd;
}
fd_pid = pid;
}
-static void RecursiveCreateParentDirs(char *path) {
+static void RecursiveCreateParentDirs(char *path, fd_t &fd) {
if (path[0] == '\0')
return;
for (int i = 1; path[i] != '\0'; ++i) {
@@ -90,7 +101,13 @@ static void RecursiveCreateParentDirs(char *path) {
WriteToFile(kStderrFd, path, internal_strlen(path));
const char *ErrorMsgSuffix = "\n";
WriteToFile(kStderrFd, ErrorMsgSuffix, internal_strlen(ErrorMsgSuffix));
- Die();
+ if (!common_flags()->log_fallback_to_stderr)
+ Die();
+ path[i] = save;
+ const char *ErrorMsgSuffix2 = "ERROR: Fallback to stderr\n";
+ WriteToFile(kStderrFd, ErrorMsgSuffix2, internal_strlen(ErrorMsgSuffix2));
+ fd = kStderrFd;
+ return;
}
path[i] = save;
}
@@ -166,7 +183,11 @@ void ReportFile::SetReportPath(const char *path) {
WriteToFile(kStderrFd, path, 8);
message = "...\n";
WriteToFile(kStderrFd, message, internal_strlen(message));
- Die();
+ if (!common_flags()->log_fallback_to_stderr)
+ Die();
+ const char *message2 = "ERROR: Fallback to stderr\n";
+ WriteToFile(kStderrFd, message2, internal_strlen(message2));
+ path = "stderr";
}
}
@@ -180,7 +201,7 @@ void ReportFile::SetReportPath(const char *path) {
fd = kStdoutFd;
} else {
ParseAndSetPath(path, path_prefix, kMaxPathLength);
- RecursiveCreateParentDirs(path_prefix);
+ RecursiveCreateParentDirs(path_prefix, fd);
}
}
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_file.h b/compiler-rt/lib/sanitizer_common/sanitizer_file.h
index bef2c842d9f24..f1a791e2aa3b1 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_file.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_file.h
@@ -43,6 +43,9 @@ struct ReportFile {
// PID of the process that opened fd. If a fork() occurs,
// the PID of child will be different from fd_pid.
uptr fd_pid;
+ // Set to true if the last attempt to open the logfile failed, perhaps due to
+ // permission errors
+ bool lastOpenFailed = false;
private:
void ReopenIfNecessary();
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc b/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
index c1e3530618c20..650a4580bbcf0 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
@@ -65,6 +65,8 @@ COMMON_FLAG(
bool, log_to_syslog, (bool)SANITIZER_ANDROID || (bool)SANITIZER_APPLE,
"Write all sanitizer output to syslog in addition to other means of "
"logging.")
+COMMON_FLAG(bool, log_fallback_to_stderr, false,
+ "When set, fallback to stderr if we are unable to open log path.")
COMMON_FLAG(
int, verbosity, 0,
"Verbosity level (0 - silent, 1 - a bit of output, 2+ - more output).")
diff --git a/compiler-rt/test/sanitizer_common/TestCases/Posix/sanitizer_set_report_path_fail.cpp b/compiler-rt/test/sanitizer_common/TestCases/Posix/sanitizer_set_report_path_fail.cpp
index af5187a0d3265..24adaaa4046c0 100644
--- a/compiler-rt/test/sanitizer_common/TestCases/Posix/sanitizer_set_report_path_fail.cpp
+++ b/compiler-rt/test/sanitizer_common/TestCases/Posix/sanitizer_set_report_path_fail.cpp
@@ -2,9 +2,11 @@
// Case 1: Try setting a path that is an invalid/inaccessible directory.
// RUN: not %run %t 2>&1 | FileCheck %s --check-prefix=ERROR1
+// RUN: %env_tool_opts=log_fallback_to_stderr=1 %run %t 2>&1 | FileCheck %s --check-prefixes=ERROR1,FALLBACK
// Case 2: Try setting a path that is too large.
// RUN: not %run %t A 2>&1 | FileCheck %s --check-prefix=ERROR2
+// RUN: %env_tool_opts=log_fallback_to_stderr=1 %run %t A 2>&1 | FileCheck %s --check-prefixes=ERROR2,FALLBACK
#include <sanitizer/common_interface_defs.h>
#include <stdio.h>
@@ -22,3 +24,4 @@ int main(int argc, char **argv) {
}
__sanitizer_set_report_path(buff);
}
+// FALLBACK: Fallback to stderr
|
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions inc,cpp,h -- compiler-rt/lib/sanitizer_common/sanitizer_file.cpp compiler-rt/lib/sanitizer_common/sanitizer_file.h compiler-rt/lib/sanitizer_common/sanitizer_flags.inc compiler-rt/test/sanitizer_common/TestCases/Posix/sanitizer_set_report_path_fail.cpp
View the diff from clang-format here.diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_file.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
index e8f219b94..916e7e79d 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
@@ -73,7 +73,7 @@ void ReportFile::ReopenIfNecessary() {
fd = OpenFile(full_path, WrOnly, &err);
if (fd == kInvalidFd) {
bool fallback = common_flags()->log_fallback_to_stderr;
- const char *ErrorMsgPrefix =
+ const char* ErrorMsgPrefix =
fallback ? "WARNING: Can't open file, falling back to stderr: "
: "ERROR: Can't open file: ";
WriteToFile(kStderrFd, ErrorMsgPrefix, internal_strlen(ErrorMsgPrefix));
@@ -89,7 +89,7 @@ void ReportFile::ReopenIfNecessary() {
fd_pid = pid;
}
-static void RecursiveCreateParentDirs(char *path, fd_t &fd) {
+static void RecursiveCreateParentDirs(char* path, fd_t& fd) {
if (path[0] == '\0')
return;
for (int i = 1; path[i] != '\0'; ++i) {
@@ -99,7 +99,7 @@ static void RecursiveCreateParentDirs(char *path, fd_t &fd) {
path[i] = '\0';
if (!DirExists(path) && !CreateDir(path)) {
bool fallback = common_flags()->log_fallback_to_stderr;
- const char *ErrorMsgPrefix =
+ const char* ErrorMsgPrefix =
fallback ? "WARNING: Can't create directory, falling back to stderr: "
: "ERROR: Can't create directory: ";
WriteToFile(kStderrFd, ErrorMsgPrefix, internal_strlen(ErrorMsgPrefix));
@@ -185,7 +185,7 @@ void ReportFile::SetReportPath(const char *path) {
uptr len = internal_strlen(path);
if (len > sizeof(path_prefix) - 100) {
bool fallback = common_flags()->log_fallback_to_stderr;
- const char *message =
+ const char* message =
fallback ? "WARNING: Path is too long, falling back to stderr: "
: "ERROR: Path is too long: ";
WriteToFile(kStderrFd, message, internal_strlen(message));
|
919a955
to
4ea07ef
Compare
4ea07ef
to
a0fbdc0
Compare
It seems that I cannot make the formatter happy. Currently the style is consistent with the rest of the file. |
if (lastOpenFailed && fd_pid != pid) { | ||
fd = kInvalidFd; | ||
lastOpenFailed = false; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment explaining what this scenario is?
Die(); | ||
if (!common_flags()->log_fallback_to_stderr) | ||
Die(); | ||
const char *errmsg2 = "ERROR: Fallback to stderr\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message seems a bit awkward, as the code continues handling the situation gracefully instead of terminating immediately.
Ideally, I want to combine the error message under this flag into something like:
ERROR: Can't open file: {blah}. Falling back to stderr.
Alternatively, we could comment with WARNING:
instead of ERROR:
in a separate message, but combining them into one line as above seems slightly better.
// process, mark fd as invalid so we attempt to open again. | ||
CHECK_EQ(fd, kStderrFd); | ||
fd = kInvalidFd; | ||
lastOpenFailed = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this flag is being cleared here. I think it would be clearer to use a more descriptive name, such as fallbackTostderrActive
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Add the santizier option
log_fallback_to_stderr
which will set the logpath tostderr
if there is an error with the provided logpath. We've seen this happen when process A has write permission to the logpath, but process B does not. In this case, we'd like process B to fallback to writing tostderr
, rather than being killed.