[libFuzzer] Diff 3 - Split IO implementation for Windows and Posix.

[mpividori]

Summary

Divide the IO functions's implementation in 2 different files for differences between POSIX and Windows API.

[amccarth]

The Windows APIs have lots of gotchas.

lib/Fuzzer/FuzzerIOWindows.cpp

28 ↗

(On Diff #79654)

This is OK as long as you know the path names will contain only characters in the user's code page (which isn't UTF-8).

If the path name being passed in is UTF-8, then you'll need to convert it to a std::wstring and call the -W versions instead of the -A versions of these file functions.

The -W versions also make it possible to deal with very long path names.

83 ↗

(On Diff #79654)

FirstFind doesn't seem like a great name, since it's also used for the subsequent files. FindInfo or something like that is probably better.

100 ↗

(On Diff #79654)

You don't need to call GetFileAttributesA. The attributes are already available in the WIN32_FIND_DATA.

102 ↗

(On Diff #79654)

If you use the attributes you already have in the WIN32_FIND_DATA, then you don't need code here to handle an error from GetFileAttributesA.

109 ↗

(On Diff #79654)

You could put the test for . and .. in this block, which should make things faster.

110 ↗

(On Diff #79654)

I generally prefer an iterative solution instead of recursive. I've found it's slightly faster to iterate one directory completely before descending to a subdirectory. It also uses fewer system resources, as you have only one FindHandle open at a time. But this is probably fine, and I don't know if the caller is expecting the files in the recursive order.

The iterative solution is to have a queue of directories. Each time you find a new directory, push it onto the queue. Iterate until the queue is empty.

[mpividori]

Update Diff.

[zturner]

Looks good to me with Adrian's suggested changes. I know you said you updated the diff, but I'm still seeing all of the issues Adrian mentioned as being unresolved?

[mpividori]

Yes, I updated the diff to move some changes from Diff 2, suggested by kcc. I need to work on the issues mentioned by Adrian.

[kcc]

Note: I rely on the other reviewers for the windows-specific parts.

lib/Fuzzer/FuzzerIO.cpp

19 ↗

(On Diff #79690)

can you avoid this ifdef?
E.g. move whatever needs unistd.h into *Posix* file

lib/Fuzzer/FuzzerIOPlatform.h

1 ↗

(On Diff #79690)

why do we need both FuzzerIO.h and FuzzerIOPlatform.h?

[mpividori]

lib/Fuzzer/FuzzerIOWindows.cpp

28 ↗

(On Diff #79654)

@amccarth thanks for your comments. Yes, we plan to modify the code in the future to use std::wstring, as Support library does.

83 ↗

(On Diff #79654)

Ok, I will change that.

100 ↗

(On Diff #79654)

@amccarth you are right. I will modify this.

109 ↗

(On Diff #79654)

@amccarth Ok.

110 ↗

(On Diff #79654)

@amccarth thanks. I adapted the previous implementation in libFuzzer for Posix, which was recursive. I don't think the order of files matters here, so I can change it to consider an iterative solution. I will add that changes in a new diff.

[mpividori]

lib/Fuzzer/FuzzerIO.cpp

19 ↗

(On Diff #79690)

@kcc Ok. I will move DeleteFile() implementation to the files FuzzerIOPosix.cpp and FuzzerIOWindows.cpp.

lib/Fuzzer/FuzzerIOPlatform.h

1 ↗

(On Diff #79690)

@kcc , The initial idea was to separate FuzzerIO.h and FuzzerIOPlatform.h, so FuzzerIO.h includes functions used by the rest of the modules, and FuzzerIOPlatform.h includes the platform specific functions only included by the FuzzerIO.cpp implementation.
But with recent changes, this is not valid any more, because some implementations need to access some functions from FuzzerIO platform specific implementation like DeleteFile(), so I will merge both headers files into FuzzerIO.h.

[mpividori]

Include changes proposed by @kcc and @amccarth .

[kcc]

LGTM

llvm/trunk/lib/Fuzzer/CMakeLists.txt

 set(LIBFUZZER_FLAGS_BASE "${CMAKE_CXX_FLAGS}")
 # Disable the coverage and sanitizer instrumentation for the fuzzer itself.
 set(CMAKE_CXX_FLAGS "${LIBFUZZER_FLAGS_BASE} -fno-sanitize=all -fno-sanitize-coverage=edge,trace-cmp,indirect-calls,8bit-counters -Werror")
 if( LLVM_USE_SANITIZE_COVERAGE )
   if(NOT "${LLVM_USE_SANITIZER}" STREQUAL "Address")
     message(FATAL_ERROR
       "LibFuzzer and its tests require LLVM_USE_SANITIZER=Address and "
       "LLVM_USE_SANITIZE_COVERAGE=YES to be set."
       )
   endif()
   add_library(LLVMFuzzerNoMainObjects OBJECT
     FuzzerCrossOver.cpp
-    FuzzerTraceState.cpp
     FuzzerDriver.cpp
     FuzzerExtFunctionsDlsym.cpp
     FuzzerExtFunctionsWeak.cpp
     FuzzerIO.cpp
+    FuzzerIOPosix.cpp
+    FuzzerIOWindows.cpp
     FuzzerLoop.cpp
     FuzzerMutate.cpp
     FuzzerSHA1.cpp
     FuzzerTracePC.cpp
+    FuzzerTraceState.cpp
     FuzzerUtil.cpp
     FuzzerUtilDarwin.cpp
     FuzzerUtilLinux.cpp
     )
   add_library(LLVMFuzzerNoMain STATIC
     $<TARGET_OBJECTS:LLVMFuzzerNoMainObjects>
     )
   target_link_libraries(LLVMFuzzerNoMain ${PTHREAD_LIB})

llvm/trunk/lib/Fuzzer/FuzzerIO.h

 //===----------------------------------------------------------------------===//
 #ifndef LLVM_FUZZER_IO_H
 #define LLVM_FUZZER_IO_H
 
 #include "FuzzerDefs.h"
 
 namespace fuzzer {
 
-bool IsFile(const std::string &Path);
-
 long GetEpoch(const std::string &Path);
 
 Unit FileToVector(const std::string &Path, size_t MaxSize = 0,
                   bool ExitOnError = true);
 
-void DeleteFile(const std::string &Path);
-
 std::string FileToString(const std::string &Path);
 
 void CopyFileToErr(const std::string &Path);
 
 void WriteToFile(const Unit &U, const std::string &Path);
 
 void ReadDirToVectorOfUnits(const char *Path, std::vector<Unit> *V,
                             long *Epoch, size_t MaxSize, bool ExitOnError);
 
 // Returns "Dir/FileName" or equivalent for the current OS.
 std::string DirPlusFile(const std::string &DirPath,
                         const std::string &FileName);
 
 void DupAndCloseStderr();
 
 void CloseStdout();
 
 void Printf(const char *Fmt, ...);
 
+// Platform specific functions:
+bool IsFile(const std::string &Path);
+
+void ListFilesInDirRecursive(const std::string &Dir, long *Epoch,
+                             std::vector<std::string> *V, bool TopDir);
+
+char GetSeparator();
+
+FILE* OpenFile(int Fd, const char *Mode);
+
+int CloseFile(int Fd);
+
+int DuplicateFile(int Fd);
+
+void DeleteFile(const std::string &Path);
+
 }  // namespace fuzzer
 #endif  // LLVM_FUZZER_IO_H

llvm/trunk/lib/Fuzzer/FuzzerIO.cpp

 //===- FuzzerIO.cpp - IO utils. -------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 // IO functions.
 //===----------------------------------------------------------------------===//
 #include "FuzzerIO.h"
 #include "FuzzerDefs.h"
 #include "FuzzerExtFunctions.h"
-#include <iterator>
+#include <algorithm>
+#include <cstdarg>
 #include <fstream>
-#include <dirent.h>
+#include <iterator>
 #include <sys/types.h>
 #include <sys/stat.h>
-#include <unistd.h>
-#include <cstdarg>
-#include <cstdio>
 
 namespace fuzzer {
 
 static FILE *OutputFile = stderr;
 
-bool IsFile(const std::string &Path) {
-  struct stat St;
-  if (stat(Path.c_str(), &St))
-    return false;
-  return S_ISREG(St.st_mode);
-}
-
 long GetEpoch(const std::string &Path) {
   struct stat St;
   if (stat(Path.c_str(), &St))
     return 0;  // Can't stat, be conservative.
   return St.st_mtime;
 }
 
-static void ListFilesInDirRecursive(const std::string &Dir, long *Epoch,
-                                    std::vector<std::string> *V, bool TopDir) {
-  auto E = GetEpoch(Dir);
-  if (Epoch)
-    if (E && *Epoch >= E) return;
-
-  DIR *D = opendir(Dir.c_str());
-  if (!D) {
-    Printf("No such directory: %s; exiting\n", Dir.c_str());
-    exit(1);
-  }
-  while (auto E = readdir(D)) {
-    std::string Path = DirPlusFile(Dir, E->d_name);
-    if (E->d_type == DT_REG || E->d_type == DT_LNK)
-      V->push_back(Path);
-    else if (E->d_type == DT_DIR && *E->d_name != '.')
-      ListFilesInDirRecursive(Path, Epoch, V, false);
-  }
-  closedir(D);
-  if (Epoch && TopDir)
-    *Epoch = E;
-}
-
 Unit FileToVector(const std::string &Path, size_t MaxSize, bool ExitOnError) {
   std::ifstream T(Path);
   if (ExitOnError && !T) {
     Printf("No such directory: %s; exiting\n", Path.c_str());
     exit(1);
   }
 
   T.seekg(0, T.end);
   size_t FileLen = T.tellg();
   if (MaxSize)
     FileLen = std::min(FileLen, MaxSize);
 
   T.seekg(0, T.beg);
   Unit Res(FileLen);
   T.read(reinterpret_cast<char *>(Res.data()), FileLen);
   return Res;
 }
 
-void DeleteFile(const std::string &Path) {
-  unlink(Path.c_str());
-}
-
 std::string FileToString(const std::string &Path) {
   std::ifstream T(Path);
   return std::string((std::istreambuf_iterator<char>(T)),
                      std::istreambuf_iterator<char>());
 }
 
 void CopyFileToErr(const std::string &Path) {
   Printf("%s", FileToString(Path).c_str());
for (size_t i = 0; i < Files.size(); i++) {
     auto S = FileToVector(X, MaxSize, ExitOnError);
     if (!S.empty())
       V->push_back(S);
   }
 }
 
 std::string DirPlusFile(const std::string &DirPath,
                         const std::string &FileName) {
-  return DirPath + "/" + FileName;
+  return DirPath + GetSeparator() + FileName;
 }
 
 void DupAndCloseStderr() {
-  int OutputFd = dup(2);
+  int OutputFd = DuplicateFile(2);
   if (OutputFd > 0) {
-    FILE *NewOutputFile = fdopen(OutputFd, "w");
+    FILE *NewOutputFile = OpenFile(OutputFd, "w");
     if (NewOutputFile) {
       OutputFile = NewOutputFile;
       if (EF->__sanitizer_set_report_fd)
         EF->__sanitizer_set_report_fd(reinterpret_cast<void *>(OutputFd));
-      close(2);
+      CloseFile(2);
     }
   }
 }
 
-void CloseStdout() { close(1); }
+void CloseStdout() {
+  CloseFile(1);
+}
 
 void Printf(const char *Fmt, ...) {
   va_list ap;
   va_start(ap, Fmt);
   vfprintf(OutputFile, Fmt, ap);
   va_end(ap);
   fflush(OutputFile);
 }
 
 }  // namespace fuzzer

llvm/trunk/lib/Fuzzer/FuzzerIOPosix.cpp

+//===- FuzzerIOPosix.cpp - IO utils for Posix. ----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+// IO functions implementation using Posix API.
+//===----------------------------------------------------------------------===//
+
+#include "FuzzerDefs.h"
+#if LIBFUZZER_POSIX
+#include "FuzzerExtFunctions.h"
+#include "FuzzerIO.h"
+#include <cstdarg>
+#include <cstdio>
+#include <dirent.h>
+#include <fstream>
+#include <iterator>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+namespace fuzzer {
+
+bool IsFile(const std::string &Path) {
+  struct stat St;
+  if (stat(Path.c_str(), &St))
+    return false;
+  return S_ISREG(St.st_mode);
+}
+
+void ListFilesInDirRecursive(const std::string &Dir, long *Epoch,
+                             std::vector<std::string> *V, bool TopDir) {
+  auto E = GetEpoch(Dir);
+  if (Epoch)
+    if (E && *Epoch >= E) return;
+
+  DIR *D = opendir(Dir.c_str());
+  if (!D) {
+    Printf("No such directory: %s; exiting\n", Dir.c_str());
+    exit(1);
+  }
+  while (auto E = readdir(D)) {
+    std::string Path = DirPlusFile(Dir, E->d_name);
+    if (E->d_type == DT_REG || E->d_type == DT_LNK)
+      V->push_back(Path);
+    else if (E->d_type == DT_DIR && *E->d_name != '.')
+      ListFilesInDirRecursive(Path, Epoch, V, false);
+  }
+  closedir(D);
+  if (Epoch && TopDir)
+    *Epoch = E;
+}
+
+char GetSeparator() {
+  return '/';
+}
+
+FILE* OpenFile(int Fd, const char* Mode) {
+  return fdopen(Fd, Mode);
+}
+
+int CloseFile(int fd) {
+  return close(fd);
+}
+
+int DuplicateFile(int Fd) {
+  return dup(Fd);
+}
+
+void DeleteFile(const std::string &Path) {
+  unlink(Path.c_str());
+}
+
+}  // namespace fuzzer
+#endif // LIBFUZZER_POSIX

llvm/trunk/lib/Fuzzer/FuzzerIOWindows.cpp

+//===- FuzzerIOWindows.cpp - IO utils for Windows. ------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+// IO functions implementation for Windows.
+//===----------------------------------------------------------------------===//
+
+#include "FuzzerDefs.h"
+#if LIBFUZZER_WINDOWS
+#include "FuzzerExtFunctions.h"
+#include "FuzzerIO.h"
+#include <cstdarg>
+#include <cstdio>
+#include <fstream>
+#include <io.h>
+#include <iterator>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <windows.h>
+
+namespace fuzzer {
+
+static bool IsFile(const std::string &Path, const DWORD &FileAttributes) {
+
+  if (FileAttributes & FILE_ATTRIBUTE_NORMAL)
+    return true;
+
+  if (FileAttributes & FILE_ATTRIBUTE_DIRECTORY)
+    return false;
+
+  HANDLE FileHandle(
+      CreateFileA(Path.c_str(), 0, FILE_SHARE_READ, NULL, OPEN_EXISTING,
+                  FILE_FLAG_BACKUP_SEMANTICS, 0));
+
+  if (FileHandle == INVALID_HANDLE_VALUE) {
+    Printf("CreateFileA() failed for \"%s\" (Error code: %lu).\n", Path.c_str(),
+        GetLastError());
+    return false;
+  }
+
+  DWORD FileType = GetFileType(FileHandle);
+
+  if (FileType == FILE_TYPE_UNKNOWN) {
+    Printf("GetFileType() failed for \"%s\" (Error code: %lu).\n", Path.c_str(),
+        GetLastError());
+    CloseHandle(FileHandle);
+    return false;
+  }
+
+  if (FileType != FILE_TYPE_DISK) {
+    CloseHandle(FileHandle);
+    return false;
+  }
+
+  CloseHandle(FileHandle);
+  return true;
+}
+
+bool IsFile(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 IsFile(Path, Att);
+}
+
+void ListFilesInDirRecursive(const std::string &Dir, long *Epoch,
+                             std::vector<std::string> *V, bool TopDir) {
+  auto E = GetEpoch(Dir);
+  if (Epoch)
+    if (E && *Epoch >= E) return;
+
+  std::string Path(Dir);
+  assert(!Path.empty());
+  if (Path.back() != '\\')
+      Path.push_back('\\');
+  Path.push_back('*');
+
+  // Get the first directory entry.
+  WIN32_FIND_DATAA FindInfo;
+  HANDLE FindHandle(FindFirstFileA(Path.c_str(), &FindInfo));
+  if (FindHandle == INVALID_HANDLE_VALUE)
+  {
+    Printf("No file found in: %s.\n", Dir.c_str());
+    return;
+  }
+
+  do {
+    std::string FileName = DirPlusFile(Dir, FindInfo.cFileName);
+
+    if (FindInfo.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) {
+      size_t FilenameLen = strlen(FindInfo.cFileName);
+      if ((FilenameLen == 1 && FindInfo.cFileName[0] == '.') ||
+          (FilenameLen == 2 && FindInfo.cFileName[0] == '.' &&
+                               FindInfo.cFileName[1] == '.'))
+        continue;
+
+      ListFilesInDirRecursive(FileName, Epoch, V, false);
+    }
+    else if (IsFile(FileName, FindInfo.dwFileAttributes))
+      V->push_back(FileName);
+  } while (FindNextFileA(FindHandle, &FindInfo));
+
+  DWORD LastError = GetLastError();
+  if (LastError != ERROR_NO_MORE_FILES)
+    Printf("FindNextFileA failed (Error code: %lu).\n", LastError);
+
+  FindClose(FindHandle);
+
+  if (Epoch && TopDir)
+    *Epoch = E;
+}
+
+char GetSeparator() {
+  return '\\';
+}
+
+FILE* OpenFile(int Fd, const char* Mode) {
+  return _fdopen(Fd, Mode);
+}
+
+int CloseFile(int Fd) {
+  return _close(Fd);
+}
+
+int DuplicateFile(int Fd) {
+  return _dup(Fd);
+}
+
+void DeleteFile(const std::string &Path) {
+  _unlink(Path.c_str());
+}
+
+}  // namespace fuzzer
+#endif // LIBFUZZER_WINDOWS