[libFuzzer] Error and exit if user supplied fuzzer writeable directories don't exist

[dgg5503]

Summary

Currently, libFuzzer will exit with an error message if a non-existent
corpus directory is provided. However, if a user provides a non-existent
directory for the artifact_prefix, exact_artifact_path, or
features_dir, libFuzzer will continue execution but silently fail to
write artifacts/features.

To improve the user experience, this PR adds validation for the existence of
all user supplied directories before executing the main fuzzing loop. If they
don't exist, libFuzzer will exit with an error message.

[dgg5503]

Applied clang-format to some areas of the code but left others alone to be consistent with the rest of the libFuzzer code base. Please let me know if it's appropriate to apply all suggested clang-tidy and clang-format changes.

[dgg5503]

Ping.

[kcc]

I'd rather fail instead of silently creating new dirs, to be consistent with the other behavior

[dgg5503]

In D84808#2194438, @kcc wrote:

I'd rather fail instead of silently creating new dirs, to be consistent with the other behavior

Are you referring to the non-existent corpus case? If so, that's also changed in this PR to have its directory created if it doesn't exist. Please let me know if I've overlooked additional cases where directories are expected to exist.

[kcc]

From the description:

this PR adds automatic directory creation for locations in which libFuzzer expects to write data.

I'd prefer libFuzzer to not create directories, but instead err-and-exit if those don't exist.

[dgg5503]

In D84808#2194844, @kcc wrote:

From the description:

this PR adds automatic directory creation for locations in which libFuzzer expects to write data.

I'd prefer libFuzzer to not create directories, but instead err-and-exit if those don't exist.

I can make this change, but is there a reason why this shouldn't be done? It seems more convenient for the end user but perhaps I'm overlooking a larger issue.

[kcc]

In D84808#2195169, @dgg5503 wrote:

In D84808#2194844, @kcc wrote:

From the description:

this PR adds automatic directory creation for locations in which libFuzzer expects to write data.

I'd prefer libFuzzer to not create directories, but instead err-and-exit if those don't exist.

I can make this change, but is there a reason why this shouldn't be done? It seems more convenient for the end user but perhaps I'm overlooking a larger issue.

First reason is consistency. We currently don't create DIRs if they don't exist.
The second reason is avoiding surprise.
Suppose a user has a typo in a directory name.
Crashing because a wrongly named dir doesn't exist is not a surprise.
Working and using a wrongly named dir *is* a surprise.

[dgg5503]

In D84808#2200407, @kcc wrote:

In D84808#2195169, @dgg5503 wrote:

In D84808#2194844, @kcc wrote:

From the description:

this PR adds automatic directory creation for locations in which libFuzzer expects to write data.

I'd prefer libFuzzer to not create directories, but instead err-and-exit if those don't exist.

I can make this change, but is there a reason why this shouldn't be done? It seems more convenient for the end user but perhaps I'm overlooking a larger issue.

First reason is consistency. We currently don't create DIRs if they don't exist.
The second reason is avoiding surprise.
Suppose a user has a typo in a directory name.
Crashing because a wrongly named dir doesn't exist is not a surprise.
Working and using a wrongly named dir *is* a surprise.

Thank you for the elaboration @kcc . Your reasoning makes sense to me given the current design of my patch. Before I make the appropriate changes, would it be acceptable to have an environment variable or launch parameter that could allow the silent creation of these directories? The reason I'm so keen on this functionality is that I'm working with a remotely accessible embedded environment where it's a bit of a pain to create these directories by hand, especially in CI situations.

[kcc]

would it be acceptable to have an environment variable or launch parameter that could allow the silent creation of these directories?

That would be a very reasonable thing to do (command line flag, not env var)!
Like, -create-nonexistent-dirs=1 (0 by default) or some such.
But please make it a separate patch to simplify the review.

[dgg5503]

Fixed the diff

[kcc]

+Matt

[dgg5503]

Ping.

[morehouse]

compiler-rt/lib/fuzzer/FuzzerDriver.cpp

692

If it is a file, should we print an error message?

708

Nit: remove this space.

[dgg5503]

compiler-rt/lib/fuzzer/FuzzerDriver.cpp

692

The first input can also be a test file rather than a directory. An example of this can be seen in fuzzer-runs.test.

I also want to point out that Options.OutputCorpus is no longer being set if a file is provided as (*Inputs)[0] unlike the old behavior. All fuzzer tests & unit tests pass on my local Linux machine so I don't think this is having any adverse effect.

[dgg5503]

Address @morehouse comment.

compiler-rt/lib/fuzzer/FuzzerDriver.cpp

   }
 }
 
+static void InitializeArgumentDirectory(const std::string &Path) {
+  if (!Path.empty() &&
+      !IsDirectory(Path) &&
+      MkDirRecursive(Path)) {
+    Printf("ERROR: Failed to create directory \"%s\"\n", Path.c_str());
+    exit(1);
+  }
+}
+
 std::string CloneArgsWithoutX(const Vector<std::string> &Args,
                               const char *X1, const char *X2) {
   std::string Cmd;

     Options.MallocLimitMb = Options.RssLimitMb;
   if (Flags.runs >= 0)
     Options.MaxNumberOfRuns = Flags.runs;
-  if (!Inputs->empty() && !Flags.minimize_crash_internal_step)
-    Options.OutputCorpus = (*Inputs)[0];
+  if (!Inputs->empty() && !Flags.minimize_crash_internal_step) {
+    // Ensure output corpus assumed to be the first arbitrary argument input
+    // is not a path to an existing file.
+    std::string OutputCorpusDir = (*Inputs)[0];
+    if (!IsFile(OutputCorpusDir)) {
+      Options.OutputCorpus = OutputCorpusDir;
+      InitializeArgumentDirectory(Options.OutputCorpus);
+    }
+  }
   Options.ReportSlowUnits = Flags.report_slow_units;
-  if (Flags.artifact_prefix)
+  if (Flags.artifact_prefix) {
     Options.ArtifactPrefix = Flags.artifact_prefix;
-  if (Flags.exact_artifact_path)
+
+    // Since the prefix could be a full path to a file name prefix, assume
+    // that if the path ends with the platform's separator that a directory
+    // is desired
+    std::string ArtifactPathDir = Options.ArtifactPrefix;
+    if (!IsSeparator(ArtifactPathDir[ArtifactPathDir.length() - 1])) {
+      ArtifactPathDir = DirName(ArtifactPathDir);
+    }
+
+    InitializeArgumentDirectory(ArtifactPathDir);
+  }
+  if (Flags.exact_artifact_path) {
     Options.ExactArtifactPath = Flags.exact_artifact_path;
+    InitializeArgumentDirectory(DirName(Options.ExactArtifactPath));
+  }
   Vector<Unit> Dictionary;
   if (Flags.dict)
     if (!ParseDictionaryFile(FileToString(Flags.dict), &Dictionary))

     Options.FocusFunction = Flags.focus_function;
   if (Flags.data_flow_trace)
     Options.DataFlowTrace = Flags.data_flow_trace;
-  if (Flags.features_dir)
+  if (Flags.features_dir) {
     Options.FeaturesDir = Flags.features_dir;
+    InitializeArgumentDirectory(Options.FeaturesDir);
+  }
   if (Flags.collect_data_flow)
     Options.CollectDataFlow = Flags.collect_data_flow;
   if (Flags.stop_file)

compiler-rt/lib/fuzzer/FuzzerIO.h

 
 // Platform specific functions:
 bool IsFile(const std::string &Path);
+bool IsDirectory(const std::string &Path);
 size_t FileSize(const std::string &Path);
 
 void ListFilesInDirRecursive(const std::string &Dir, long *Epoch,
                              Vector<std::string> *V, bool TopDir);
 
+int MkDirRecursive(const std::string &Dir);
 void RmDirRecursive(const std::string &Dir);
 
 // Iterate files and dirs inside Dir, recursively.

 void GetSizedFilesFromDir(const std::string &Dir, Vector<SizedFile> *V);
 
 char GetSeparator();
+bool IsSeparator(char C);
 // Similar to the basename utility: returns the file name w/o the dir prefix.
 std::string Basename(const std::string &Path);
 

 
 intptr_t GetHandleFromFd(int fd);
 
-void MkDir(const std::string &Path);
+int MkDir(const std::string &Path);
 void RmDir(const std::string &Path);
 
 const std::string &getDevNull();

compiler-rt/lib/fuzzer/FuzzerIO.cpp

   fflush(OutputFile);
 }
 
+static int MkDirRecursiveInner(const std::string &Dir, const std::string &Leaf) {
+  if (IsDirectory(Dir)) {
+    return MkDir(Leaf);
+  }
+
+  int ret = MkDirRecursiveInner(DirName(Dir), Dir);
+  if (ret) {
+     // Give up early if a previous MkDir failed
+     return ret;
+  }
+
+  return MkDir(Leaf);
+}
+
+int MkDirRecursive(const std::string &Dir) {
+  if (Dir.empty())
+    return -1;
+  return MkDirRecursiveInner(DirName(Dir), Dir);
+}
+
 void RmDirRecursive(const std::string &Dir) {
   IterateDirRecursive(
       Dir, [](const std::string &Path) {},

compiler-rt/lib/fuzzer/FuzzerIOPosix.cpp

   return S_ISREG(St.st_mode);
 }
 
-static bool IsDirectory(const std::string &Path) {
+bool IsDirectory(const std::string &Path) {
   struct stat St;
   if (stat(Path.c_str(), &St))
     return false;

   return '/';
 }
 
+bool IsSeparator(char C) {
+  return C == '/';
+}
+
 FILE* OpenFile(int Fd, const char* Mode) {
   return fdopen(Fd, Mode);
 }

   write(2, Str, strlen(Str));
 }
 
-void MkDir(const std::string &Path) {
-  mkdir(Path.c_str(), 0700);
+int MkDir(const std::string &Path) {
+  return mkdir(Path.c_str(), 0700);
 }
 
 void RmDir(const std::string &Path) {

compiler-rt/lib/fuzzer/FuzzerIOWindows.cpp

   return FileAttrs & FILE_ATTRIBUTE_DIRECTORY;
 }
 
+bool IsDirectory(const std::string &Path) {
+  DWORD Att = GetFileAttributesA(Path.c_str());
+
+  if (Att == INVALID_FILE_ATTRIBUTES) {
+    Printf("GetFileAttributesA() failed for \"%s\" (Error code: %lu).\n",
+        Path.c_str(), GetLastError());
+    return false;
+  }
+
+  return IsDir(Att);
+}
+
 std::string Basename(const std::string &Path) {
   size_t Pos = Path.find_last_of("/\\");
   if (Pos == std::string::npos) return Path;

   return _get_osfhandle(fd);
 }
 
-static bool IsSeparator(char C) {
+bool IsSeparator(char C) {
   return C == '\\' || C == '/';
 }
 

   _write(2, Str, strlen(Str));
 }
 
-void MkDir(const std::string &Path) {
-  if (CreateDirectoryA(Path.c_str(), nullptr)) return;
+int MkDir(const std::string &Path) {
+  if (CreateDirectoryA(Path.c_str(), nullptr)) return 0;
   Printf("CreateDirectoryA failed for %s (Error code: %lu).\n", Path.c_str(),
          GetLastError());
+  return -1;
 }
 
 void RmDir(const std::string &Path) {

compiler-rt/test/fuzzer/fuzzer-dirs.test

 LONG: INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 8192 bytes
 RUN: rm -rf %t/SUB1
 
-RUN: not %run %t-SimpleTest NONEXISTENT_DIR 2>&1 | FileCheck %s --check-prefix=NONEXISTENT_DIR
-NONEXISTENT_DIR: No such file or directory: NONEXISTENT_DIR; exiting
+# For platforms without functioning chmod (i.e. Windows), use a forbidden
+# character in the directory name.
+RUN: rm -rf %t.dir && mkdir -p %t.dir/access_restricted && chmod u-w %t.dir/access_restricted
+RUN: not %run %t-SimpleTest %t.dir/access_restricted/?corpus? 2>&1 | FileCheck %s --check-prefix=DIR_CREATION_FAILURE
+DIR_CREATION_FAILURE: ERROR: Failed to create directory {{.*/access_restricted/\?corpus\?}}
 
+RUN: rm -rf %t.dir && mkdir -p %t.dir
+RUN: not %run %t-SimpleTest -artifact_prefix=%t.dir/subdira/artifacts/ -features_dir=%t.dir/subdirb/features/ %t.dir/subdirc/corpus
+RUN: test -e %t.dir/subdira/artifacts/
+RUN: test -e %t.dir/subdirb/features/
+RUN: test -e %t.dir/subdirc/corpus/
+
+RUN: not %run %t-SimpleTest -exact_artifact_path=%t.dir/subdird/exact_artifacts/abc
+RUN: test -e %t.dir/subdird/exact_artifacts/abc
+
+RUN: not %run %t-SimpleTest -artifact_prefix=%t.dir/subdire/myprefix
+RUN: test -e %t.dir/subdire/ && not test -e %t.dir/subdire/myprefix