Abstract fd operations better in tool.

Windows and POSIX implement very similar fd operations, but differ
slightly:

- ssize_t in POSIX is usually int on Windows.
- POSIX needs EINTR retry loops.
- Windows wants _open rather than open, etc.
- POSIX fds and sockets are the same thing, while Windows sockets are
  HANDLEs and leaves fd as a C runtime construct.

Rather than ad-hoc macros and redefinitions of ssize_t (which reportedly
upset MINGW), add some actual abstractions. While I'm here, add a scoped
file descriptor type.

That still leaves recv/send which are only used in one file, so defined
a socket_result_t for them.

Change-Id: I17fca2a50c77191f573852bfd27553996e3e9c3f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/41725
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Adam Langley <[email protected]>

Author: davidben

tool/CMakeLists.txt

@@ -8,6 +8,7 @@ add_executable(
   client.cc
   const.cc
   digest.cc
+  fd.cc
   file.cc
   generate_ed25519.cc
   genrsa.cc

tool/digest.cc

@@ -37,27 +37,13 @@ OPENSSL_MSVC_PRAGMA(warning(push, 3))
 OPENSSL_MSVC_PRAGMA(warning(pop))
 #include <io.h>
 #define PATH_MAX MAX_PATH
-typedef int ssize_t;
 #endif
 
 #include <openssl/digest.h>
 
 #include "internal.h"
 
 
-struct close_delete {
-  void operator()(int *fd) {
-    BORINGSSL_CLOSE(*fd);
-  }
-};
-
-template<typename T, typename R, R (*func) (T*)>
-struct func_delete {
-  void operator()(T* obj) {
-    func(obj);
-  }
-};
-
 // Source is an awkward expression of a union type in C++: Stdin | File filename.
 struct Source {
   enum Type {
@@ -79,37 +65,31 @@ struct Source {
 
 static const char kStdinName[] = "standard input";
 
-// OpenFile opens the regular file named |filename| and sets |*out_fd| to be a
-// file descriptor to it. Returns true on sucess or prints an error to stderr
-// and returns false on error.
-static bool OpenFile(int *out_fd, const std::string &filename) {
-  *out_fd = -1;
-
-  int fd = BORINGSSL_OPEN(filename.c_str(), O_RDONLY | O_BINARY);
-  if (fd < 0) {
+// OpenFile opens the regular file named |filename| and returns a file
+// descriptor to it.
+static ScopedFD OpenFile(const std::string &filename) {
+  ScopedFD fd = OpenFD(filename.c_str(), O_RDONLY | O_BINARY);
+  if (!fd) {
     fprintf(stderr, "Failed to open input file '%s': %s\n", filename.c_str(),
             strerror(errno));
-    return false;
+    return ScopedFD();
   }
-  std::unique_ptr<int, close_delete> scoped_fd(&fd);
 
 #if !defined(OPENSSL_WINDOWS)
   struct stat st;
-  if (fstat(fd, &st)) {
+  if (fstat(fd.get(), &st)) {
     fprintf(stderr, "Failed to stat input file '%s': %s\n", filename.c_str(),
             strerror(errno));
-    return false;
+    return ScopedFD();
   }
 
   if (!S_ISREG(st.st_mode)) {
     fprintf(stderr, "%s: not a regular file\n", filename.c_str());
-    return false;
+    return ScopedFD();
   }
 #endif
 
-  *out_fd = fd;
-  scoped_fd.release();
-  return true;
+  return fd;
 }
 
 // SumFile hashes the contents of |source| with |md| and sets |*out_hex| to the
@@ -119,16 +99,17 @@ static bool OpenFile(int *out_fd, const std::string &filename) {
 // error.
 static bool SumFile(std::string *out_hex, const EVP_MD *md,
                     const Source &source) {
-  std::unique_ptr<int, close_delete> scoped_fd;
+  ScopedFD scoped_fd;
   int fd;
 
   if (source.is_stdin()) {
     fd = 0;
   } else {
-    if (!OpenFile(&fd, source.filename())) {
+    scoped_fd = OpenFile(source.filename());
+    if (!scoped_fd) {
       return false;
     }
-    scoped_fd.reset(&fd);
+    fd = scoped_fd.get();
   }
 
   static const size_t kBufSize = 8192;
@@ -141,21 +122,18 @@ static bool SumFile(std::string *out_hex, const EVP_MD *md,
   }
 
   for (;;) {
-    ssize_t n;
-
-    do {
-      n = BORINGSSL_READ(fd, buf.get(), kBufSize);
-    } while (n == -1 && errno == EINTR);
-
-    if (n == 0) {
-      break;
-    } else if (n < 0) {
+    size_t n;
+    if (!ReadFromFD(fd, &n, buf.get(), kBufSize)) {
       fprintf(stderr, "Failed to read from %s: %s\n",
               source.is_stdin() ? kStdinName : source.filename().c_str(),
               strerror(errno));
       return false;
     }
 
+    if (n == 0) {
+      break;
+    }
+
     if (!EVP_DigestUpdate(ctx.get(), buf.get(), n)) {
       fprintf(stderr, "Failed to update hash.\n");
       return false;
@@ -221,25 +199,23 @@ struct CheckModeArguments {
 // returns false.
 static bool Check(const CheckModeArguments &args, const EVP_MD *md,
                   const Source &source) {
-  std::unique_ptr<FILE, func_delete<FILE, int, fclose>> scoped_file;
   FILE *file;
+  ScopedFILE scoped_file;
 
   if (source.is_stdin()) {
     file = stdin;
   } else {
-    int fd;
-    if (!OpenFile(&fd, source.filename())) {
+    ScopedFD fd = OpenFile(source.filename());
+    if (!fd) {
       return false;
     }
 
-    file = BORINGSSL_FDOPEN(fd, "rb");
-    if (!file) {
+    scoped_file = FDToFILE(std::move(fd), "rb");
+    if (!scoped_file) {
       perror("fdopen");
-      BORINGSSL_CLOSE(fd);
       return false;
     }
-
-    scoped_file = std::unique_ptr<FILE, func_delete<FILE, int, fclose>>(file);
+    file = scoped_file.get();
   }
 
   const size_t hex_size = EVP_MD_size(md) * 2;

tool/fd.cc

@@ -0,0 +1,105 @@
+/* Copyright (c) 2020, Google Inc.
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
+ * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
+ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */
+
+#include <openssl/base.h>
+
+#include <errno.h>
+#include <limits.h>
+#include <stdio.h>
+
+#include <algorithm>
+
+#include "internal.h"
+
+#if defined(OPENSSL_WINDOWS)
+#include <io.h>
+#else
+#include <fcntl.h>
+#include <unistd.h>
+#endif
+
+
+ScopedFD OpenFD(const char *path, int flags) {
+#if defined(OPENSSL_WINDOWS)
+  return ScopedFD(_open(path, flags));
+#else
+  int fd;
+  do {
+    fd = open(path, flags);
+  } while (fd == -1 && errno == EINTR);
+  return ScopedFD(fd);
+#endif
+}
+
+void CloseFD(int fd) {
+#if defined(OPENSSL_WINDOWS)
+  _close(fd);
+#else
+  close(fd);
+#endif
+}
+
+bool ReadFromFD(int fd, size_t *out_bytes_read, void *out, size_t num) {
+#if defined(OPENSSL_WINDOWS)
+  // On Windows, the buffer must be at most |INT_MAX|. See
+  // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/read?view=vs-2019
+  int ret = _read(fd, out, std::min(size_t{INT_MAX}, num));
+#else
+  ssize_t ret;
+  do {
+    ret = read(fd, out, num);
+  } while (ret == -1 && errno == EINVAL);
+#endif
+
+  if (ret < 0) {
+    *out_bytes_read = 0;
+    return false;
+  }
+  *out_bytes_read = ret;
+  return true;
+}
+
+bool WriteToFD(int fd, size_t *out_bytes_written, const void *in, size_t num) {
+#if defined(OPENSSL_WINDOWS)
+  // The documentation for |_write| does not say the buffer must be at most
+  // |INT_MAX|, but clamp it to |INT_MAX| instead of |UINT_MAX| in case.
+  int ret = _write(fd, in, std::min(size_t{INT_MAX}, num));
+#else
+  ssize_t ret;
+  do {
+    ret = write(fd, in, num);
+  } while (ret == -1 && errno == EINVAL);
+#endif
+
+  if (ret < 0) {
+    *out_bytes_written = 0;
+    return false;
+  }
+  *out_bytes_written = ret;
+  return true;
+}
+
+ScopedFILE FDToFILE(ScopedFD fd, const char *mode) {
+  ScopedFILE ret;
+#if defined(OPENSSL_WINDOWS)
+  ret.reset(_fdopen(fd.get(), mode));
+#else
+  ret.reset(fdopen(fd.get(), mode));
+#endif
+  // |fdopen| takes ownership of |fd| on success.
+  if (ret) {
+    fd.release();
+  }
+  return ret;
+}

tool/internal.h

@@ -18,32 +18,17 @@
 #include <openssl/base.h>
 
 #include <string>
+#include <utility>
 #include <vector>
 
-OPENSSL_MSVC_PRAGMA(warning(push))
 // MSVC issues warning C4702 for unreachable code in its xtree header when
 // compiling with -D_HAS_EXCEPTIONS=0. See
 // https://connect.microsoft.com/VisualStudio/feedback/details/809962
+OPENSSL_MSVC_PRAGMA(warning(push))
 OPENSSL_MSVC_PRAGMA(warning(disable: 4702))
-
 #include <map>
-
 OPENSSL_MSVC_PRAGMA(warning(pop))
 
-#if defined(OPENSSL_WINDOWS)
-  #define BORINGSSL_OPEN _open
-  #define BORINGSSL_FDOPEN _fdopen
-  #define BORINGSSL_CLOSE _close
-  #define BORINGSSL_READ _read
-  #define BORINGSSL_WRITE _write
-#else
-  #define BORINGSSL_OPEN open
-  #define BORINGSSL_FDOPEN fdopen
-  #define BORINGSSL_CLOSE close
-  #define BORINGSSL_READ read
-  #define BORINGSSL_WRITE write
-#endif
-
 struct FileCloser {
   void operator()(FILE *file) {
     fclose(file);
@@ -52,6 +37,67 @@ struct FileCloser {
 
 using ScopedFILE = std::unique_ptr<FILE, FileCloser>;
 
+// The following functions abstract between POSIX and Windows differences in
+// file descriptor I/O functions.
+
+// CloseFD behaves like |close|.
+void CloseFD(int fd);
+
+class ScopedFD {
+ public:
+  ScopedFD() {}
+  explicit ScopedFD(int fd) : fd_(fd) {}
+  ScopedFD(ScopedFD &&other) { *this = std::move(other); }
+  ScopedFD(const ScopedFD &) = delete;
+  ~ScopedFD() { reset(); }
+
+  ScopedFD &operator=(const ScopedFD &) = delete;
+  ScopedFD &operator=(ScopedFD &&other) {
+    reset();
+    fd_ = other.fd_;
+    other.fd_ = -1;
+    return *this;
+  }
+
+  explicit operator bool() const { return fd_ >= 0; }
+
+  int get() const { return fd_; }
+
+  void reset() {
+    if (fd_ >= 0) {
+      CloseFD(fd_);
+    }
+    fd_ = -1;
+  }
+
+  int release() {
+    int fd = fd_;
+    fd_ = -1;
+    return fd;
+  }
+
+ private:
+  int fd_ = -1;
+};
+
+// OpenFD behaves like |open| but handles |EINTR| and works on Windows.
+ScopedFD OpenFD(const char *path, int flags);
+
+// ReadFromFD reads up to |num| bytes from |fd| and writes the result to |out|.
+// On success, it returns true and sets |*out_bytes_read| to the number of bytes
+// read. Otherwise, it returns false and leaves an error in |errno|. On POSIX,
+// it handles |EINTR| internally.
+bool ReadFromFD(int fd, size_t *out_bytes_read, void *out, size_t num);
+
+// WriteToFD writes up to |num| bytes from |in| to |fd|. On success, it returns
+// true and sets |*out_bytes_written| to the number of bytes written. Otherwise,
+// it returns false and leaves an error in |errno|. On POSIX, it handles |EINTR|
+// internally.
+bool WriteToFD(int fd, size_t *out_bytes_written, const void *in, size_t num);
+
+// FDToFILE behaves like |fdopen|.
+ScopedFILE FDToFILE(ScopedFD fd, const char *mode);
+
 enum ArgumentType {
   kRequiredArgument,
   kOptionalArgument,