[Sanitizer] Allow setting the report path to create directory

[tejohnson]

Summary

When setting the report path, recursively create the directory as
needed. This brings the profile path support for memprof on par with
normal PGO. The code was largely cloned from __llvm_profile_recursive_mkdir
in compiler-rt/lib/profile/InstrProfilingUtil.c.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_file.cpp

87

to avoid #ifs here can you please create CreateDir() and provide two implementations:
in sanitizer_win and sanitizer_posix

93

please clang-format as it asks

[tejohnson]

Move mkdir calls to OS specific helper.

[tejohnson]

Undo formatting change in unrelated existing code

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_common.h

263 ↗

(On Diff #372633)

can we avoid #if?
we don't have one e.g. for GetProcessName

compiler-rt/lib/sanitizer_common/sanitizer_file.cpp

78

Maybe we can add ReportFile::SetReportPath unittest here:
sanitizer_common/tests/sanitizer_common_test.cpp

The function is little bit unconventional and does not create path for the last component. Maybe rename into something RecursiveCreateParentDirs(char* path)

82

We don't need two vars outside the loop.
Also if we start with 1 we can overflow on empty string.

84

if (!IsPathSeparator) ?

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp

154

int -> bool, to avoid exposing platform specific error errors.

[tejohnson]

compiler-rt/lib/sanitizer_common/sanitizer_common.h

263 ↗

(On Diff #372633)

GetProcessName has a single def in sanitizer_common.cpp. A closer analogy would be Abort(), which has implementations in all of these 4 places:
sanitizer_posix_libcdep.cpp
sanitizer_common_nolibc.cpp
sanitizer_win.cpp
sanitizer_fuchsia.cpp

Since CreateDir calls mkdir/_mkdir it also needs to be defined in similar places, and I have defs in the first 3 of the above. I believe for things to link properly on Fuchsia I would need to provide one there too. However, sanitizer_file.cpp which uses this new function has its code entirely guarded by #if !SANITIZER_FUCHSIA, so I wasn't sure it made sense. WDYT?

compiler-rt/lib/sanitizer_common/sanitizer_file.cpp

78

I ended up adding a new unittest to sanitizer_common/tests/sanitizer_libc_test.cpp instead, since it is already testing some sanitizer_file interfaces, and it probably only makes sense to test this under libc where we can mkdir anyway.

82

We start at 1 because if the path starts with "/" it doesn't make sense to try to mkdir an empty string. I added a check for the path being empty above this loop.

84

Woops, yes, I introduced this during some cleanup before sending the patch.

[tejohnson]

Address comments

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_common.h

263 ↗

(On Diff #372633)

I guess you can just move CreateDir next to IsPathSeparator into sanitizer_file.h without any defines. That header is better place for the definition anyway.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_common.h

263 ↗

(On Diff #372633)

"without any defines" -> "without any ifdefs"

[tejohnson]

Address comments

[morehouse]

compiler-rt/lib/sanitizer_common/sanitizer_win.cpp

568

This line caused a build breakage on the sanitizer bot: https://lab.llvm.org/buildbot/#/builders/127/builds/17222

C:\...\sanitizer_win.cpp(568): error C3861: '_mkdir': identifier not found

Could you please take a look?

[tejohnson]

compiler-rt/lib/sanitizer_common/sanitizer_win.cpp

568

Sorry, missed that one. Hopefully 1864976c967de36146eb5a5b86e6312d466e1031 that I just committed should take care of it.

[denik]

This change breaks tests with sanitizers running in a sandbox.
If /usr/local/tmp/asan is in the allow-list for writing, the sandbox will kill the test when it tries to create /usr.

This failure is caught on Chrome OS, https://issuetracker.google.com/209296420.

[tejohnson]

In D109794#3309768, @denik wrote:

This change breaks tests with sanitizers running in a sandbox.
If /usr/local/tmp/asan is in the allow-list for writing, the sandbox will kill the test when it tries to create /usr.

This failure is caught on Chrome OS, https://issuetracker.google.com/209296420.

It sounds like the issue is showing up with some Chrome OS tests built with -fsanitize=address, as opposed to in the unit test added here - is that correct?

If the former, by default the report_file is stderr - do you know where/how the report file path is being set to something else? Note the change only creates a directory for the given report_file path, the sanitizer change itself doesn't change the path. Before the report file creation would fail if the directory didn't exist.

If the latter, we can probably avoid running the unit test in that environment - there are other tests that create temp files so there must be an existing mechanism to skip them or do something different.

[denik]

In D109794#3309991, @tejohnson wrote:

In D109794#3309768, @denik wrote:

This change breaks tests with sanitizers running in a sandbox.
If /usr/local/tmp/asan is in the allow-list for writing, the sandbox will kill the test when it tries to create /usr.

This failure is caught on Chrome OS, https://issuetracker.google.com/209296420.

It sounds like the issue is showing up with some Chrome OS tests built with -fsanitize=address, as opposed to in the unit test added here - is that correct?

Correct, I meant Chrome OS tests. Chrome OS uses Gentoo sandboxing for its unit tests.

If the former, by default the report_file is stderr - do you know where/how the report file path is being set to something else? Note the change only creates a directory for the given report_file path, the sanitizer change itself doesn't change the path. Before the report file creation would fail if the directory didn't exist.

log_path is set in https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/profiles/base/profile.bashrc#512
Note that the directory was created in the same function. And it's called before entering a sandbox.

Can we skip CreateDir in RecursiveCreateParentDirs if the path exists? So it will be no-op in our case?

[tejohnson]

In D109794#3310024, @denik wrote:

In D109794#3309991, @tejohnson wrote:

In D109794#3309768, @denik wrote:

This change breaks tests with sanitizers running in a sandbox.
If /usr/local/tmp/asan is in the allow-list for writing, the sandbox will kill the test when it tries to create /usr.

This failure is caught on Chrome OS, https://issuetracker.google.com/209296420.

It sounds like the issue is showing up with some Chrome OS tests built with -fsanitize=address, as opposed to in the unit test added here - is that correct?

Correct, I meant Chrome OS tests. Chrome OS uses Gentoo sandboxing for its unit tests.

If the former, by default the report_file is stderr - do you know where/how the report file path is being set to something else? Note the change only creates a directory for the given report_file path, the sanitizer change itself doesn't change the path. Before the report file creation would fail if the directory didn't exist.

log_path is set in https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/profiles/base/profile.bashrc#512
Note that the directory was created in the same function. And it's called before entering a sandbox.

Can we skip CreateDir in RecursiveCreateParentDirs if the path exists? So it will be no-op in our case?

Oh I see, we normally assume it will error (but we ignore the error) if the directory already exists. But in the sandboxed case it doesn't like the creation attempt, even though the dir actually exists.

Yeah, we can presumably use "stat" to check before trying to create. Actually, there is already a FileExists() in sanitizer_file.h that has implementations in the various OS specific sanitizer files. Most (except windows) call stat (but look for a regular file). I can just add something like a DirExists() interface that has the equivalent dir check using the same interfaces in each OS implementation. Will work on that tomorrow.

[tejohnson]

In D109794#3310049, @tejohnson wrote:

In D109794#3310024, @denik wrote:

In D109794#3309991, @tejohnson wrote:

In D109794#3309768, @denik wrote:

This change breaks tests with sanitizers running in a sandbox.
If /usr/local/tmp/asan is in the allow-list for writing, the sandbox will kill the test when it tries to create /usr.

This failure is caught on Chrome OS, https://issuetracker.google.com/209296420.

It sounds like the issue is showing up with some Chrome OS tests built with -fsanitize=address, as opposed to in the unit test added here - is that correct?

Correct, I meant Chrome OS tests. Chrome OS uses Gentoo sandboxing for its unit tests.

If the former, by default the report_file is stderr - do you know where/how the report file path is being set to something else? Note the change only creates a directory for the given report_file path, the sanitizer change itself doesn't change the path. Before the report file creation would fail if the directory didn't exist.

log_path is set in https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/profiles/base/profile.bashrc#512
Note that the directory was created in the same function. And it's called before entering a sandbox.

Can we skip CreateDir in RecursiveCreateParentDirs if the path exists? So it will be no-op in our case?

Oh I see, we normally assume it will error (but we ignore the error) if the directory already exists. But in the sandboxed case it doesn't like the creation attempt, even though the dir actually exists.

Yeah, we can presumably use "stat" to check before trying to create. Actually, there is already a FileExists() in sanitizer_file.h that has implementations in the various OS specific sanitizer files. Most (except windows) call stat (but look for a regular file). I can just add something like a DirExists() interface that has the equivalent dir check using the same interfaces in each OS implementation. Will work on that tomorrow.

Are you able to patch in D119495 and see if that fixes the issue?

[denik]

In D109794#3312880, @tejohnson wrote:

In D109794#3310049, @tejohnson wrote:

In D109794#3310024, @denik wrote:

In D109794#3309991, @tejohnson wrote:

In D109794#3309768, @denik wrote:

This change breaks tests with sanitizers running in a sandbox.
If /usr/local/tmp/asan is in the allow-list for writing, the sandbox will kill the test when it tries to create /usr.

This failure is caught on Chrome OS, https://issuetracker.google.com/209296420.

It sounds like the issue is showing up with some Chrome OS tests built with -fsanitize=address, as opposed to in the unit test added here - is that correct?

Correct, I meant Chrome OS tests. Chrome OS uses Gentoo sandboxing for its unit tests.

If the former, by default the report_file is stderr - do you know where/how the report file path is being set to something else? Note the change only creates a directory for the given report_file path, the sanitizer change itself doesn't change the path. Before the report file creation would fail if the directory didn't exist.

log_path is set in https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/profiles/base/profile.bashrc#512
Note that the directory was created in the same function. And it's called before entering a sandbox.

Can we skip CreateDir in RecursiveCreateParentDirs if the path exists? So it will be no-op in our case?

Oh I see, we normally assume it will error (but we ignore the error) if the directory already exists. But in the sandboxed case it doesn't like the creation attempt, even though the dir actually exists.

Yeah, we can presumably use "stat" to check before trying to create. Actually, there is already a FileExists() in sanitizer_file.h that has implementations in the various OS specific sanitizer files. Most (except windows) call stat (but look for a regular file). I can just add something like a DirExists() interface that has the equivalent dir check using the same interfaces in each OS implementation. Will work on that tomorrow.

Are you able to patch in D119495 and see if that fixes the issue?

Yes. D119495 fixed the problem. Thanks for the quick solution!

Just curious, did __llvm_profile_recursive_mkdir also create directories in a similar way (ignoring EEXIST status)? I'm asking because the same test was creating a PGO profile in a sandbox and it did not fail.
While debugging the sandbox (before your fix), I noticed that the test crashed when the sandbox was checking lstat of the path from __sanitizer::CreateDir(). Can it be that the sanitizer intercepted the call but since it was __asan::AsanInitInternal() the handler was not set up yet? This would explain why it worked for PGO.

[tejohnson]

In D109794#3313433, @denik wrote:

In D109794#3312880, @tejohnson wrote:

In D109794#3310049, @tejohnson wrote:

In D109794#3310024, @denik wrote:

In D109794#3309991, @tejohnson wrote:

In D109794#3309768, @denik wrote:

This change breaks tests with sanitizers running in a sandbox.
If /usr/local/tmp/asan is in the allow-list for writing, the sandbox will kill the test when it tries to create /usr.

This failure is caught on Chrome OS, https://issuetracker.google.com/209296420.

It sounds like the issue is showing up with some Chrome OS tests built with -fsanitize=address, as opposed to in the unit test added here - is that correct?

Correct, I meant Chrome OS tests. Chrome OS uses Gentoo sandboxing for its unit tests.

If the former, by default the report_file is stderr - do you know where/how the report file path is being set to something else? Note the change only creates a directory for the given report_file path, the sanitizer change itself doesn't change the path. Before the report file creation would fail if the directory didn't exist.

log_path is set in https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/profiles/base/profile.bashrc#512
Note that the directory was created in the same function. And it's called before entering a sandbox.

Can we skip CreateDir in RecursiveCreateParentDirs if the path exists? So it will be no-op in our case?

Oh I see, we normally assume it will error (but we ignore the error) if the directory already exists. But in the sandboxed case it doesn't like the creation attempt, even though the dir actually exists.

Yeah, we can presumably use "stat" to check before trying to create. Actually, there is already a FileExists() in sanitizer_file.h that has implementations in the various OS specific sanitizer files. Most (except windows) call stat (but look for a regular file). I can just add something like a DirExists() interface that has the equivalent dir check using the same interfaces in each OS implementation. Will work on that tomorrow.

Are you able to patch in D119495 and see if that fixes the issue?

Yes. D119495 fixed the problem. Thanks for the quick solution!

Great, will try to get this submitted today, once I address the comments.

Just curious, did __llvm_profile_recursive_mkdir also create directories in a similar way (ignoring EEXIST status)? I'm asking because the same test was creating a PGO profile in a sandbox and it did not fail.

Interesting - I actually modeled the implementation here exactly on that function. It has the same unconditional mkdir with the same comment about ignoring failures due to existing directories.

While debugging the sandbox (before your fix), I noticed that the test crashed when the sandbox was checking lstat of the path from __sanitizer::CreateDir(). Can it be that the sanitizer intercepted the call but since it was __asan::AsanInitInternal() the handler was not set up yet? This would explain why it worked for PGO.

Not totally following - which handler?

[denik]

In D109794#3314322, @tejohnson wrote:

In D109794#3313433, @denik wrote:

Are you able to patch in D119495 and see if that fixes the issue?

Yes. D119495 fixed the problem. Thanks for the quick solution!

Great, will try to get this submitted today, once I address the comments.

Just curious, did __llvm_profile_recursive_mkdir also create directories in a similar way (ignoring EEXIST status)? I'm asking because the same test was creating a PGO profile in a sandbox and it did not fail.

Interesting - I actually modeled the implementation here exactly on that function. It has the same unconditional mkdir with the same comment about ignoring failures due to existing directories.

While debugging the sandbox (before your fix), I noticed that the test crashed when the sandbox was checking lstat of the path from __sanitizer::CreateDir(). Can it be that the sanitizer intercepted the call but since it was __asan::AsanInitInternal() the handler was not set up yet? This would explain why it worked for PGO.

Not totally following - which handler?

I meant the sanitizer interceptor: https://github.com/llvm-mirror/compiler-rt/blob/master/lib/sanitizer_common/sanitizer_common_interceptors.inc#L6510
With llvm debug info enabled:

(gdb) bt
#0  __interceptor_lstat ()
    at /var/tmp/portage/sys-devel/llvm-14.0_pre437112_p20211208-r7/work/llvm-14.0_pre437112_p20211208/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:6878
#1  0x00007ffff7f9e297 in sb_mkdirat_pre_check (func=0x7ffff7f963e0 "mkdir", pathname=0x31bfdc <__sanitizer::report_file+12> "/build",
    dirfd=-100) at /var/tmp/portage/sys-apps/sandbox-2.29/work/sandbox-2.29/libsandbox/pre_check_mkdirat.c:40
#2  0x00007ffff7fa167b in mkdir_DEFAULT (pathname=0x31bfdc <__sanitizer::report_file+12> "/build", mode=493)
    at /var/tmp/portage/sys-apps/sandbox-2.29/work/sandbox-2.29/libsandbox/wrapper-funcs/__wrapper_simple.c:51
#3  0x00000000002faaab in __sanitizer::CreateDir(char const*) ()
    at /var/tmp/portage/sys-devel/llvm-14.0_pre437112_p20211208-r7/work/llvm-14.0_pre437112_p20211208/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp:154
#4  0x00000000002eaab6 in RecursiveCreateParentDirs ()
    at /var/tmp/portage/sys-devel/llvm-14.0_pre437112_p20211208-r7/work/llvm-14.0_pre437112_p20211208/compiler-rt/lib/sanitizer_common/sanitizer_file.cpp:87
#5  SetReportPath ()
    at /var/tmp/portage/sys-devel/llvm-14.0_pre437112_p20211208-r7/work/llvm-14.0_pre437112_p20211208/compiler-rt/lib/sanitizer_common/sanitizer_file.cpp:112
#6  0x00000000002e03b5 in AsanInitInternal ()
    at /var/tmp/portage/sys-devel/llvm-14.0_pre437112_p20211208-r7/work/llvm-14.0_pre437112_p20211208/compiler-rt/lib/asan/asan_rtl.cpp:426
(gdb) frame 0
#0  __interceptor_lstat ()
    at /var/tmp/portage/sys-devel/llvm-14.0_pre437112_p20211208-r7/work/llvm-14.0_pre437112_p20211208/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:6878
6878      COMMON_INTERCEPTOR_ENTER(ctx, lstat, path, buf);
(gdb) s

Program received signal SIGSEGV, Segmentation fault.
0x0000000000000000 in ?? ()

It crashes on the COMMON_INTERCEPTOR_ENTER() call.

compiler-rt/lib/sanitizer_common/sanitizer_common_nolibc.cpp

 // The Windows implementations of these functions use the win32 API directly,
 // bypassing libc.
 #if !SANITIZER_WINDOWS
 #if SANITIZER_LINUX
 void LogMessageOnPrintf(const char *str) {}
 #endif
 void WriteToSyslog(const char *buffer) {}
 void Abort() { internal__exit(1); }
+bool CreateDir(const char *pathname) { return false; }
 #endif // !SANITIZER_WINDOWS
 
 #if !SANITIZER_WINDOWS && !SANITIZER_MAC
 void ListOfModules::init() {}
 void InitializePlatformCommonFlags(CommonFlags *cf) {}
 #endif
 
 }  // namespace __sanitizer

compiler-rt/lib/sanitizer_common/sanitizer_file.h

 bool SupportsColoredOutput(fd_t fd);
 
 // OS
 const char *GetPwd();
 bool FileExists(const char *filename);
 char *FindPathToBinary(const char *name);
 bool IsPathSeparator(const char c);
 bool IsAbsolutePath(const char *path);
+// Returns true on success, false on failure.
+bool CreateDir(const char *pathname);
 // Starts a subprocess and returs its pid.
 // If *_fd parameters are not kInvalidFd their corresponding input/output
 // streams will be redirect to the file. The files will always be closed
 // in parent process even in case of an error.
 // The child process will close all fds after STDERR_FILENO
 // before passing control to a program.
 pid_t StartSubprocess(const char *filename, const char *const argv[],
                       const char *const envp[], fd_t stdin_fd = kInvalidFd,

compiler-rt/lib/sanitizer_common/sanitizer_file.cpp

if (fd == kInvalidFd) {
     char errmsg[100];
     internal_snprintf(errmsg, sizeof(errmsg), " (reason: %d)", err);
     WriteToFile(kStderrFd, errmsg, internal_strlen(errmsg));
     Die();
   }
   fd_pid = pid;
 }
 
+static void RecursiveCreateParentDirs(char *path) {
+  if (path[0] == '\0')
+    return;
+  for (int i = 1; path[i] != '\0'; ++i) {
+    char save = path[i];
+    if (!IsPathSeparator(path[i]))
+      continue;
+    path[i] = '\0';
+    /* Some of these will fail, because the directory exists, ignore it. */
+    CreateDir(path);
+    path[i] = save;
+  }
+}
+
 void ReportFile::SetReportPath(const char *path) {
   if (path) {
     uptr len = internal_strlen(path);
     if (len > sizeof(path_prefix) - 100) {
       Report("ERROR: Path is too long: %c%c%c%c%c%c%c%c...\n", path[0], path[1],
              path[2], path[3], path[4], path[5], path[6], path[7]);
       Die();
     }
   }
 
   SpinMutexLock l(mu);
   if (fd != kStdoutFd && fd != kStderrFd && fd != kInvalidFd)
     CloseFile(fd);
   fd = kInvalidFd;
   if (!path || internal_strcmp(path, "stderr") == 0) {
     fd = kStderrFd;
   } else if (internal_strcmp(path, "stdout") == 0) {
     fd = kStdoutFd;
   } else {
     internal_snprintf(path_prefix, kMaxPathLength, "%s", path);
+    RecursiveCreateParentDirs(path_prefix);
   }
 }
 
 const char *ReportFile::GetReportPath() {
   SpinMutexLock l(mu);
   ReopenIfNecessary();
   return full_path;
 }

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp

 int Atexit(void (*function)(void)) {
 #if !SANITIZER_GO
   return atexit(function);
 #else
   return 0;
 #endif
 }
 
+bool CreateDir(const char *pathname) { return mkdir(pathname, 0755) == 0; }
+
 bool SupportsColoredOutput(fd_t fd) {
   return isatty(fd) != 0;
 }
 
 #if !SANITIZER_GO
 // TODO(glider): different tools may require different altstack size.
 static uptr GetAltStackSize() {
   // Note: since GLIBC_2.31, SIGSTKSZ may be a function call, so this may be

compiler-rt/lib/sanitizer_common/sanitizer_win.cpp

 }
 
 u64 MonotonicNanoTime() { return NanoTime(); }
 
 void Abort() {
   internal__exit(3);
 }
 
+bool CreateDir(const char *pathname) { return _mkdir(pathname) == 0; }
+
 #if !SANITIZER_GO
 // Read the file to extract the ImageBase field from the PE header. If ASLR is
 // disabled and this virtual address is available, the loader will typically
 // load the image at this address. Therefore, we call it the preferred base. Any
 // addresses in the DWARF typically assume that the object has been loaded at
 // this address.
 static uptr GetPreferredBase(const char *modname, char *buf, size_t buf_size) {
   fd_t fd = OpenFile(modname, RdOnly, nullptr);

compiler-rt/lib/sanitizer_common/tests/sanitizer_libc_test.cpp

TEST(SanitizerCommon, InternalMmapWithOffset) {
   ASSERT_EQ('A', p[0]);
   ASSERT_EQ('B', p[1]);
 
   CloseFile(fd);
   UnmapOrDie(p, page_size);
   internal_unlink(tmpfile);
 }
 #endif
+
+TEST(SanitizerCommon, ReportFile) {
+  StaticSpinMutex report_file_mu;
+  ReportFile report_file = {&report_file_mu, kStderrFd, "", "", 0};
+  char tmpfile[128];
+  temp_file_name(tmpfile, sizeof(tmpfile),
+                 "dir/sanitizer_common.reportfile.tmp.");
+  report_file.SetReportPath(tmpfile);
+  const char *path = report_file.GetReportPath();
+  EXPECT_EQ(internal_strncmp(tmpfile, path, strlen(tmpfile)), 0);
+  // This will close tmpfile.
+  report_file.SetReportPath("stderr");
+  Unlink(tmpfile);
+}

compiler-rt/test/sanitizer_common/TestCases/Posix/sanitizer_set_report_path_test.cpp

 // Test __sanitizer_set_report_path and __sanitizer_get_report_path:
 // RUN: %clangxx -O2 %s -o %t
 // RUN: %run %t | FileCheck %s
 
 #include <assert.h>
 #include <sanitizer/common_interface_defs.h>
 #include <stdio.h>
 #include <string.h>
 
 volatile int *null = 0;
 
 int main(int argc, char **argv) {
   char buff[1000];
-  sprintf(buff, "%s.report_path", argv[0]);
+  sprintf(buff, "%s.report_path/report", argv[0]);
   __sanitizer_set_report_path(buff);
   assert(strncmp(buff, __sanitizer_get_report_path(), strlen(buff)) == 0);
   printf("Path %s\n", __sanitizer_get_report_path());
 }
 
-// CHECK: Path {{.*}}Posix/Output/sanitizer_set_report_path_test.cpp.tmp.report_path.
+// CHECK: Path {{.*}}Posix/Output/sanitizer_set_report_path_test.cpp.tmp.report_path/report.