Add xReadfile wrapper for reading small to medium size files

Inspired by proposed Linux syscall

Avoid file descriptor leaks like 4af8c63f

XUtils.c

@@ -10,6 +10,7 @@ in the source distribution for its full text.
 #include "XUtils.h"
 
 #include <assert.h>
+#include <errno.h>
 #include <stdarg.h>
 #include <stdlib.h>
 #include <string.h>
@@ -210,3 +211,52 @@ char* xStrndup(const char* str, size_t len) {
    }
    return data;
 }
+
+static ssize_t readfd_internal(int fd, void* buffer, size_t count) {
+   if (!count) {
+      close(fd);
+      return -EINVAL;
+   }
+
+   ssize_t alreadyRead = 0;
+   count--; // reserve one for null-terminator
+
+   for (;;) {
+      ssize_t res = read(fd, buffer, count);
+      if (res == -1) {
+         if (errno == EINTR)
+            continue;
+
+         close(fd);
+         return -errno;
+      }
+
+      if (res > 0) {
+         buffer = ((char*)buffer) + res;
+         count -= (size_t)res;
+         alreadyRead += res;
+      }
+
+      if (count == 0 || res == 0) {
+         close(fd);
+         *((char*)buffer) = '\0';
+         return alreadyRead;
+      }
+   }
+}
+
+ssize_t xReadfile(const char* pathname, void* buffer, size_t count) {
+   int fd = open(pathname, O_RDONLY);
+   if (fd < 0)
+      return -errno;
+
+   return readfd_internal(fd, buffer, count);
+}
+
+ssize_t xReadfileat(openat_arg_t dirfd, const char* pathname, void* buffer, size_t count) {
+   int fd = Compat_openat(dirfd, pathname, O_RDONLY);
+   if (fd < 0)
+      return -errno;
+
+   return readfd_internal(fd, buffer, count);
+}

XUtils.h

@@ -14,6 +14,7 @@ in the source distribution for its full text.
 #include <stdlib.h> // IWYU pragma: keep
 #include <string.h> // IWYU pragma: keep
 
+#include "Compat.h"
 #include "Macros.h"
 
 
@@ -63,4 +64,7 @@ char* xStrdup(const char* str) ATTR_NONNULL;
 
 char* xStrndup(const char* str, size_t len) ATTR_NONNULL;
 
+ssize_t xReadfile(const char* pathname, void* buffer, size_t count);
+ssize_t xReadfileat(openat_arg_t dirfd, const char* pathname, void* buffer, size_t count);
+
 #endif

linux/LinuxProcessList.c

@@ -58,29 +58,6 @@ in the source distribution for its full text.
 #endif
 
 
-static ssize_t xread(int fd, void* buf, size_t count) {
-   // Read some bytes. Retry on EINTR and when we don't get as many bytes as we requested.
-   size_t alreadyRead = 0;
-   for (;;) {
-      ssize_t res = read(fd, buf, count);
-      if (res == -1) {
-         if (errno == EINTR)
-            continue;
-         return -1;
-      }
-
-      if (res > 0) {
-         buf = ((char*)buf) + res;
-         count -= res;
-         alreadyRead += res;
-      }
-
-      if (count == 0 || res == 0) {
-         return alreadyRead;
-      }
-   }
-}
-
 static FILE* fopenat(openat_arg_t openatArg, const char* pathname, const char* mode) {
    assert(String_eq(mode, "r")); /* only currently supported mode */
 
@@ -108,28 +85,12 @@ static int sortTtyDrivers(const void* va, const void* vb) {
 
 static void LinuxProcessList_initTtyDrivers(LinuxProcessList* this) {
    TtyDriver* ttyDrivers;
-   int fd = open(PROCTTYDRIVERSFILE, O_RDONLY);
+
-   if (fd == -1)
+   char buf[16384];
+   ssize_t r = xReadfile(PROCTTYDRIVERSFILE, buf, sizeof(buf));
+   if (r < 0)
       return;
 
-   char* buf = NULL;
-   int bufSize = MAX_READ;
-   int bufLen = 0;
-   for (;;) {
-      buf = xRealloc(buf, bufSize);
-      int size = xread(fd, buf + bufLen, MAX_READ);
-      if (size <= 0) {
-         buf[bufLen] = '\0';
-         close(fd);
-         break;
-      }
-      bufLen += size;
-      bufSize += MAX_READ;
-   }
-   if (bufLen == 0) {
-      free(buf);
-      return;
-   }
    int numDrivers = 0;
    int allocd = 10;
    ttyDrivers = xMalloc(sizeof(TtyDriver) * allocd);
@@ -169,7 +130,6 @@ static void LinuxProcessList_initTtyDrivers(LinuxProcessList* this) {
          ttyDrivers = xRealloc(ttyDrivers, sizeof(TtyDriver) * allocd);
       }
    }
-   free(buf);
    numDrivers++;
    ttyDrivers = xRealloc(ttyDrivers, sizeof(TtyDriver) * numDrivers);
    ttyDrivers[numDrivers - 1].path = NULL;
@@ -325,17 +285,11 @@ static bool LinuxProcessList_readStatFile(Process* process, openat_arg_t procFd,
    LinuxProcess* lp = (LinuxProcess*) process;
    const int commLenIn = *commLen;
    *commLen = 0;
-   int fd = Compat_openat(procFd, "stat", O_RDONLY);
-   if (fd == -1)
-      return false;
 
-   static char buf[MAX_READ + 1];
+   char buf[MAX_READ + 1];
-
+   ssize_t r = xReadfileat(procFd, "stat", buf, sizeof(buf));
-   int size = xread(fd, buf, MAX_READ);
+   if (r < 0)
-   close(fd);
-   if (size <= 0)
       return false;
-   buf[size] = '\0';
 
    assert(process->pid == atoi(buf));
    char* location = strchr(buf, ' ');
@@ -425,8 +379,9 @@ static bool LinuxProcessList_statProcessDir(Process* process, openat_arg_t procF
 }
 
 static void LinuxProcessList_readIoFile(LinuxProcess* process, openat_arg_t procFd, unsigned long long now) {
-   int fd = Compat_openat(procFd, "io", O_RDONLY);
+   char buffer[1024];
-   if (fd == -1) {
+   ssize_t r = xReadfileat(procFd, "io", buffer, sizeof(buffer));
+   if (r < 0) {
       process->io_rate_read_bps = NAN;
       process->io_rate_write_bps = NAN;
       process->io_rchar = -1LL;
@@ -441,13 +396,6 @@ static void LinuxProcessList_readIoFile(LinuxProcess* process, openat_arg_t proc
       return;
    }
 
-   char buffer[1024];
-   ssize_t buflen = xread(fd, buffer, 1023);
-   close(fd);
-   if (buflen < 1)
-      return;
-
-   buffer[buflen] = '\0';
    unsigned long long last_read = process->io_read_bytes;
    unsigned long long last_write = process->io_write_bytes;
    char* buf = buffer;
@@ -1015,14 +963,10 @@ static void setCommand(Process* process, const char* command, int len) {
 }
 
 static bool LinuxProcessList_readCmdlineFile(Process* process, openat_arg_t procFd) {
-   LinuxProcess *lp = (LinuxProcess *)process;
-   int fd = Compat_openat(procFd, "cmdline", O_RDONLY);
-   if (fd == -1)
-      return false;
-
    char command[4096 + 1]; // max cmdline length on Linux
-   int amtRead = xread(fd, command, sizeof(command) - 1);
+   ssize_t amtRead = xReadfileat(procFd, "cmdline", command, sizeof(command));
-   close(fd);
+   if (amtRead < 0)
+      return false;
 
    if (amtRead == 0) {
       if (process->state == 'Z') {
@@ -1031,8 +975,6 @@ static bool LinuxProcessList_readCmdlineFile(Process* process, openat_arg_t proc
          ((LinuxProcess*)process)->isKernelThread = true;
       }
       return true;
-   } else if (amtRead < 0) {
-      return false;
    }
 
    int tokenEnd = 0;
@@ -1145,6 +1087,7 @@ static bool LinuxProcessList_readCmdlineFile(Process* process, openat_arg_t proc
       tokenEnd = lastChar + 1;
    }
 
+   LinuxProcess *lp = (LinuxProcess *)process;
    lp->mergedCommand.maxLen = lastChar + 1;  /* accommodate cmdline */
    if (!process->comm || !String_eq(command, process->comm)) {
       process->basenameOffset = tokenEnd;
@@ -1155,9 +1098,8 @@ static bool LinuxProcessList_readCmdlineFile(Process* process, openat_arg_t proc
    }
 
    /* /proc/[pid]/comm could change, so should be updated */
-   if ((fd = Compat_openat(procFd, "comm", O_RDONLY)) != -1 &&
+   if ((amtRead = xReadfileat(procFd, "comm", command, sizeof(command))) > 0) {
-       (amtRead = xread(fd, command, sizeof(command) - 1)) > 0) {
+      command[amtRead - 1] = '\0';
-      command[amtRead - 1] = 0;
       lp->mergedCommand.maxLen += amtRead - 1;  /* accommodate comm */
       if (!lp->procComm || !String_eq(command, lp->procComm)) {
          free(lp->procComm);
@@ -1170,9 +1112,6 @@ static bool LinuxProcessList_readCmdlineFile(Process* process, openat_arg_t proc
       lp->mergedCommand.commChanged = true;
    }
 
-   if (fd != -1)
-      close(fd);
-
    char filename[MAX_NAME + 1];
 
    /* execve could change /proc/[pid]/exe, so procExe should be updated */

linux/Platform.c

@@ -717,28 +717,6 @@ static void Platform_Battery_getProcData(double* percent, ACPresence* isOnAC) {
 // READ FROM /sys
 // ----------------------------------------
 
-static inline ssize_t xread(int fd, void* buf, size_t count) {
-   // Read some bytes. Retry on EINTR and when we don't get as many bytes as we requested.
-   size_t alreadyRead = 0;
-   for (;;) {
-      ssize_t res = read(fd, buf, count);
-      if (res == -1) {
-         if (errno == EINTR)
-            continue;
-         return -1;
-      }
-
-      if (res > 0) {
-         buf = ((char*)buf) + res;
-         count -= res;
-         alreadyRead += res;
-      }
-
-      if (count == 0 || res == 0)
-         return alreadyRead;
-   }
-}
-
 static void Platform_Battery_getSysData(double* percent, ACPresence* isOnAC) {
 
    *percent = NAN;
@@ -760,31 +738,22 @@ static void Platform_Battery_getSysData(double* percent, ACPresence* isOnAC) {
       char filePath[256];
 
       xSnprintf(filePath, sizeof filePath, SYS_POWERSUPPLY_DIR "/%s/type", entryName);
-      int fd1 = open(filePath, O_RDONLY);
-      if (fd1 == -1)
-         continue;
 
       char type[8];
-      ssize_t typelen = xread(fd1, type, 7);
+      ssize_t r = xReadfile(filePath, type, sizeof(type));
-      close(fd1);
+      if (r < 3)
-      if (typelen < 1)
          continue;
 
       if (type[0] == 'B' && type[1] == 'a' && type[2] == 't') {
          xSnprintf(filePath, sizeof filePath, SYS_POWERSUPPLY_DIR "/%s/uevent", entryName);
-         int fd2 = open(filePath, O_RDONLY);
+
-         if (fd2 == -1) {
-            closedir(dir);
-            return;
-         }
          char buffer[1024];
-         ssize_t buflen = xread(fd2, buffer, 1023);
+         r = xReadfile(filePath, buffer, sizeof(buffer));
-         close(fd2);
+         if (r < 0) {
-         if (buflen < 1) {
             closedir(dir);
             return;
          }
-         buffer[buflen] = '\0';
+
          char* buf = buffer;
          char* line = NULL;
          bool full = false;
@@ -836,19 +805,15 @@ static void Platform_Battery_getSysData(double* percent, ACPresence* isOnAC) {
             continue;
 
          xSnprintf(filePath, sizeof filePath, SYS_POWERSUPPLY_DIR "/%s/online", entryName);
-         int fd3 = open(filePath, O_RDONLY);
+
-         if (fd3 == -1) {
+         char buffer[2];
+
+         r = xReadfile(filePath, buffer, sizeof(buffer));
+         if (r < 1) {
             closedir(dir);
             return;
          }
-         char buffer[2] = "";
+
-         for (;;) {
-            ssize_t res = read(fd3, buffer, 1);
-            if (res == -1 && errno == EINTR)
-               continue;
-            break;
-         }
-         close(fd3);
          if (buffer[0] == '0')
             *isOnAC = AC_ABSENT;
          else if (buffer[0] == '1')

linux/SELinuxMeter.c

@@ -58,17 +58,10 @@ static bool isSelinuxEnforcing(void) {
       return false;
    }
 
-   int fd = open("/sys/fs/selinux/enforce", O_RDONLY);
+   char buf[20];
-   if (fd < 0) {
+   ssize_t r = xReadfile("/sys/fs/selinux/enforce", buf, sizeof(buf));
+   if (r < 0)
       return false;
-   }
-
-   char buf[20] = {0};
-   int r = read(fd, buf, sizeof(buf) - 1);
-   close(fd);
-   if (r < 0) {
-      return false;
-   }
 
    int enforce = 0;
    if (sscanf(buf, "%d", &enforce) != 1) {