Harden payload buffer bounds handling

.bazelrc

@@ -5,6 +5,9 @@ build --cxxopt="-std=c++17"
 build:apple_silicon --cpu=darwin_arm64
 build:apple_silicon --features=oso_prefix_is_pwd
 
+# Strict diagnostics for this repository's C++ sources.
+build:strict --per_file_copt=toolbelt/.*@-Wall,-Wextra,-Wpedantic,-Wconversion,-Wsign-conversion,-Wshadow,-Wnon-virtual-dtor,-Wold-style-cast,-Wcast-align,-Woverloaded-virtual,-Wnull-dereference,-Wdouble-promotion,-Wformat=2,-Wimplicit-fallthrough,-Wundef,-Wextra-semi,-Wcast-qual,-Wmissing-declarations,-Wheader-hygiene,-Wthread-safety,-Wcomma,-Wrange-loop-analysis,-Wdeprecated,-Werror,-Wno-nullability-extension,-Wno-gcc-compat,-Wno-unknown-warning-option
+
 # -----------------------------------------------------------------------------
 # Sanitizer / dynamic-analysis configurations.
 #

toolbelt/BUILD.bazel

@@ -20,6 +20,7 @@ cc_library(
         "bitset.h",
         "clock.h",
         "color.h",
+        "coroutine.h",
         "fd.h",
         "hexdump.h",
         "logging.h",

toolbelt/fd.cc

@@ -1,14 +1,21 @@
 #include "toolbelt/fd.h"
 
+#include <limits>
+
 namespace toolbelt {
 
 // Close all open file descriptor for which the predicate returns true.
 void CloseAllFds(std::function<bool(int)> predicate) {
   struct rlimit lim;
   int e = getrlimit(RLIMIT_NOFILE, &lim);
   if (e == 0) {
-    for (rlim_t fd = 0; fd < lim.rlim_cur; fd++) {
-      if (fcntl(fd, F_GETFD) == 0 && predicate(fd) ) {
+    const rlim_t int_max = static_cast<rlim_t>(std::numeric_limits<int>::max());
+    for (rlim_t i = 0; i < lim.rlim_cur; ++i) {
+      if (i > int_max) {
+        break;
+      }
+      const int fd = static_cast<int>(i);
+      if (fcntl(fd, F_GETFD) == 0 && predicate(fd)) {
         (void)close(fd);
       }
     }
@@ -17,6 +24,9 @@ void CloseAllFds(std::function<bool(int)> predicate) {
 
 absl::StatusOr<ssize_t> FileDescriptor::Read(void *buffer, size_t length,
                                              const co::Coroutine *c) {
+  if (length > static_cast<size_t>(std::numeric_limits<ssize_t>::max())) {
+    return absl::InternalError("Read size too large");
+  }
   char *buf = reinterpret_cast<char *>(buffer);
   size_t total = 0;
   while (total < length) {
@@ -52,13 +62,16 @@ absl::StatusOr<ssize_t> FileDescriptor::Read(void *buffer, size_t length,
       return absl::InternalError(
           absl::StrFormat("Read failed: %s", strerror(errno)));
     }
-    total += n;
+    total += static_cast<size_t>(n);
   }
-  return total;
+  return static_cast<ssize_t>(total);
 }
 
 absl::StatusOr<ssize_t> FileDescriptor::Write(const void *buffer, size_t length,
                                               const co::Coroutine *c) {
+  if (length > static_cast<size_t>(std::numeric_limits<ssize_t>::max())) {
+    return absl::InternalError("Write size too large");
+  }
   const char *buf = reinterpret_cast<const char *>(buffer);
 
   size_t total = 0;
@@ -95,9 +108,9 @@ absl::StatusOr<ssize_t> FileDescriptor::Write(const void *buffer, size_t length,
       return absl::InternalError(
           absl::StrFormat("Write failed: %s", strerror(errno)));
     }
-    total += n;
+    total += static_cast<size_t>(n);
   }
-  return total;
+  return static_cast<ssize_t>(total);
 }
 
 } // namespace toolbelt

toolbelt/fd.h

@@ -18,7 +18,7 @@
 #include <sys/resource.h>
 #include <sys/stat.h>
 #include <unistd.h>
-#include "co/coroutine.h"
+#include "toolbelt/coroutine.h"
 
 namespace toolbelt {
 
@@ -93,7 +93,7 @@ class FileDescriptor {
   bool IsATTY() const { return Valid() && isatty(data_->fd); }
 
   // Current reference count.
-  int RefCount() const { return data_ == nullptr ? 0 : data_.use_count(); }
+  long RefCount() const { return data_ == nullptr ? 0 : data_.use_count(); }
 
   // Construct and return a struct pollfd suitable for use in ::poll.
   struct pollfd GetPollFd() {

toolbelt/hexdump.cc

@@ -11,23 +11,26 @@ namespace toolbelt {
 
 void Hexdump(const void *addr, size_t length, FILE* out) {
   const char *p = reinterpret_cast<const char *>(addr);
-  length = (length + 15) & ~15;
   while (length > 0) {
-    fprintf(out, "%p ", p);
-    for (int i = 0; i < 16; i++) {
-     fprintf(out, "%02X ", p[i] & 0xff);
+    const size_t row_length = length < 16U ? length : 16U;
+    fprintf(out, "%p ", static_cast<const void *>(p));
+    for (size_t i = 0; i < row_length; i++) {
+      fprintf(out, "%02X ", static_cast<unsigned char>(p[i]) & 0xffU);
+    }
+    for (size_t i = row_length; i < 16U; ++i) {
+      fprintf(out, "   ");
     }
     fprintf(out, "  ");
-    for (int i = 0; i < 16; i++) {
-      if (isprint(p[i])) {
+    for (size_t i = 0; i < row_length; i++) {
+      if (isprint(static_cast<unsigned char>(p[i]))) {
         fprintf(out, "%c", p[i]);
       } else {
         fprintf(out, ".");
       }
     }
     fprintf(out, "\n");
-    p += 16;
-    length -= 16;
+    p += row_length;
+    length -= row_length;
   }
 }
 

toolbelt/logging.cc

@@ -5,6 +5,7 @@
 #include "logging.h"
 #include "absl/strings/str_format.h"
 #include "clock.h"
+#include <algorithm>
 #include <cstdio>
 #include <inttypes.h>
 #include <termios.h>
@@ -156,28 +157,38 @@ void Logger::VLog(LogLevel level, const char *fmt, va_list ap) {
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wformat-nonliteral"
 #endif
-  size_t n = vsnprintf(buffer_, sizeof(buffer_), fmt, ap);
+  const int formatted = vsnprintf(buffer_, sizeof(buffer_), fmt, ap);
 #if defined(__clang__)
 #pragma clang diagnostic pop
 #elif defined(__GNUC__)
 #pragma GCC diagnostic pop
 #endif
 
+  const size_t n =
+      formatted < 0
+          ? size_t{0}
+          : std::min(static_cast<size_t>(formatted), sizeof(buffer_) - 1);
+  if (formatted < 0) {
+    buffer_[0] = '\0';
+  }
+
   // Strip final \n if present.  Refactoring from printf can leave
   // this in place.
-  if (buffer_[n - 1] == '\n') {
+  if (n > 0 && buffer_[n - 1] == '\n') {
     buffer_[n - 1] = '\0';
   }
 
   struct timespec now_ts;
   clock_gettime(CLOCK_REALTIME, &now_ts);
-  uint64_t now_ns = now_ts.tv_sec * 1000000000LL + now_ts.tv_nsec;
+  uint64_t now_ns = static_cast<uint64_t>(now_ts.tv_sec) * 1000000000ULL +
+                    static_cast<uint64_t>(now_ts.tv_nsec);
 
   char timebuf[64];
   struct tm tm;
-  n = strftime(timebuf, sizeof(timebuf), "%Y-%m-%d %H:%M:%S",
+  const size_t time_length =
+      strftime(timebuf, sizeof(timebuf), "%Y-%m-%d %H:%M:%S",
                localtime_r(&now_ts.tv_sec, &tm));
-  snprintf(timebuf + n, sizeof(timebuf) - n, ".%09" PRIu64,
+  snprintf(timebuf + time_length, sizeof(timebuf) - time_length, ".%09" PRIu64,
            now_ns % 1000000000);
 
   Log(level, now_ns, "", buffer_);
@@ -191,13 +202,13 @@ void Logger::Log(LogLevel level, uint64_t timestamp, const std::string &source,
 
   // Strip final \n if present.  Refactoring from printf can leave
   // this in place.
-  if (text[text.size() - 1] == '\n') {
+  if (!text.empty() && text.back() == '\n') {
     text = text.substr(0, text.size() - 1);
   }
 
   char timebuf[64];
   struct tm tm;
-  time_t secs = timestamp / 1000000000LL;
+  time_t secs = static_cast<time_t>(timestamp / 1000000000ULL);
   size_t n = strftime(timebuf, sizeof(timebuf), "%Y-%m-%d %H:%M:%S",
                       localtime_r(&secs, &tm));
   snprintf(timebuf + n, sizeof(timebuf) - n, ".%09" PRIu64,
@@ -245,20 +256,17 @@ void Logger::SetDisplayMode(int fd) {
       column_widths_[0] = 30; // Timestamp.
 
       // Subsystem, with a max of 20.
-      column_widths_[1] = int(subsystem_.size());
-      if (column_widths_[1] > 20) {
-        column_widths_[1] = 20;
-      }
+      column_widths_[1] = std::min(subsystem_.size(), size_t{20});
       column_widths_[2] = 3;  // Log level
       column_widths_[3] = 20; // Source
-      ssize_t remaining = screen_width_;
-      for (int i = 0; i < 4; i++) {
-        remaining -= column_widths_[i] + 1;
+      ssize_t remaining = static_cast<ssize_t>(screen_width_);
+      for (size_t i = 0; i < 4; i++) {
+        remaining -= static_cast<ssize_t>(column_widths_[i] + 1);
       }
-      if (remaining < 0) {
+      if (remaining <= 1) {
         remaining = 20;
       }
-      column_widths_[4] = remaining - 1;
+      column_widths_[4] = static_cast<size_t>(remaining - 1);
       display_mode_ = LogDisplayMode::kColumnar;
     }
   } else {
@@ -313,8 +321,8 @@ void Logger::LogColumnar(const char *timebuf, LogLevel level,
   bool first_line = true;
   size_t start = 0;
   int prefix_length = 0;
-  for (int i = 0; i < 4; i++) {
-    prefix_length += column_widths_[i] + 1;
+  for (size_t i = 0; i < 4; i++) {
+    prefix_length += static_cast<int>(column_widths_[i]) + 1;
   }
   for (;;) {
     std::string segment = text.substr(start);
@@ -326,23 +334,23 @@ void Logger::LogColumnar(const char *timebuf, LogLevel level,
     if (segment.size() > column_widths_[4]) {
       segment = segment.substr(0, column_widths_[4]);
       // Move back to the first space to avoid splitting words.
-      ssize_t i = segment.size() - 1;
+      ssize_t i = static_cast<ssize_t>(segment.size()) - 1;
       while (i > 0) {
-        if (isspace(segment[i])) {
+        if (isspace(segment[static_cast<size_t>(i)])) {
           break;
         }
         i--;
       }
       // If there is no space we just split the word.
       if (i != 0) {
-        segment = segment.substr(0, i);
+        segment = segment.substr(0, static_cast<size_t>(i));
       }
     }
     // clang-format off.
     fprintf(output_stream_, "%-*s%s%-*s%s\n", prefix_length,
             first_line ? prefix.c_str() : "",
             color::SetColor(ColorForLogLevel(level)).c_str(),
-            int(column_widths_[4]), segment.c_str(),
+            static_cast<int>(column_widths_[4]), segment.c_str(),
             color::ResetColor().c_str());
     // clang-format on
     start += segment.size();

toolbelt/manual_socket_programs/network_receiver.cc

@@ -17,7 +17,11 @@ int main(int argc, char **argv) {
   if (protocol == "tcp") {
     addr = toolbelt::InetAddress::AnyAddress(port);
   } else if (protocol == "vm") {
-    addr = toolbelt::VirtualAddress::AnyAddress(port);
+    if (port < 0) {
+      std::cerr << "VM port must be non-negative" << std::endl;
+      return 1;
+    }
+    addr = toolbelt::VirtualAddress::AnyAddress(static_cast<uint32_t>(port));
   } else {
     std::cerr << "Unknown protocol: " << protocol << std::endl;
     return 1;
@@ -68,7 +72,9 @@ int main(int argc, char **argv) {
       return 1;
     }
     std::cerr << "Received " << *status_or
-              << " bytes: " << std::string(message, *status_or) << std::endl;
+              << " bytes: "
+              << std::string(message, static_cast<size_t>(*status_or))
+              << std::endl;
   }
 
   return 0;

toolbelt/manual_socket_programs/network_sender.cc

@@ -1,5 +1,7 @@
 #include "toolbelt/sockets.h"
 
+#include <limits>
+
 int main(int argc, char *argv[]) {
   // TCP socket sender.
   // 3 args:
@@ -20,7 +22,13 @@ int main(int argc, char *argv[]) {
   if (protocol == "tcp") {
     addr = toolbelt::InetAddress(address, port);
   } else if (protocol == "vm") {
-    addr = toolbelt::VirtualAddress(std::atoi(address.c_str()), port);
+    const unsigned long cid = std::stoul(address);
+    if (cid > std::numeric_limits<uint32_t>::max() || port < 0) {
+      std::cerr << "VM CID and port must fit in uint32_t" << std::endl;
+      return 1;
+    }
+    addr = toolbelt::VirtualAddress(static_cast<uint32_t>(cid),
+                                    static_cast<uint32_t>(port));
   } else {
     std::cerr << "Unknown protocol: " << protocol << std::endl;
     return 1;

toolbelt/manual_socket_programs/tcp_receiver.cc

@@ -45,7 +45,9 @@ int main(int argc, char **argv) {
       return 1;
     }
     std::cerr << "Received " << *status_or
-              << " bytes: " << std::string(message, *status_or) << std::endl;
+              << " bytes: "
+              << std::string(message, static_cast<size_t>(*status_or))
+              << std::endl;
   }
 
   return 0;