From f7a89529330044f4e2a38e85a88ec90f839ae64e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Wed, 25 Nov 2020 12:42:36 +0100 Subject: Add xReadfile wrapper for reading small to medium size files Inspired by proposed Linux syscall Avoid file descriptor leaks like 4af8c63f --- XUtils.c | 50 ++++++++++++++++++++++++++ XUtils.h | 4 +++ linux/LinuxProcessList.c | 93 +++++++++--------------------------------------- linux/Platform.c | 59 +++++++----------------------- linux/SELinuxMeter.c | 13 ++----- 5 files changed, 85 insertions(+), 134 deletions(-) diff --git a/XUtils.c b/XUtils.c index dcc8b390..23ec39e7 100644 --- a/XUtils.c +++ b/XUtils.c @@ -10,6 +10,7 @@ in the source distribution for its full text. #include "XUtils.h" #include +#include #include #include #include @@ -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); +} diff --git a/XUtils.h b/XUtils.h index a978a0ed..d004785f 100644 --- a/XUtils.h +++ b/XUtils.h @@ -14,6 +14,7 @@ in the source distribution for its full text. #include // IWYU pragma: keep #include // 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 diff --git a/linux/LinuxProcessList.c b/linux/LinuxProcessList.c index 98b2262c..fd8f5aff 100644 --- a/linux/LinuxProcessList.c +++ b/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) - 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); + char buf[16384]; + ssize_t r = xReadfile(PROCTTYDRIVERSFILE, buf, sizeof(buf)); + if (r < 0) 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]; - - int size = xread(fd, buf, MAX_READ); - close(fd); - if (size <= 0) + char buf[MAX_READ + 1]; + ssize_t r = xReadfileat(procFd, "stat", buf, sizeof(buf)); + if (r < 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); - if (fd == -1) { + char buffer[1024]; + 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); - close(fd); + ssize_t amtRead = xReadfileat(procFd, "cmdline", command, sizeof(command)); + 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 && - (amtRead = xread(fd, command, sizeof(command) - 1)) > 0) { - command[amtRead - 1] = 0; + if ((amtRead = xReadfileat(procFd, "comm", command, sizeof(command))) > 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 */ diff --git a/linux/Platform.c b/linux/Platform.c index ff4fcf69..1462f82e 100644 --- a/linux/Platform.c +++ b/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); - close(fd1); - if (typelen < 1) + ssize_t r = xReadfile(filePath, type, sizeof(type)); + if (r < 3) 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); - close(fd2); - if (buflen < 1) { + r = xReadfile(filePath, buffer, sizeof(buffer)); + if (r < 0) { 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') diff --git a/linux/SELinuxMeter.c b/linux/SELinuxMeter.c index ee1d8470..4bbf030c 100644 --- a/linux/SELinuxMeter.c +++ b/linux/SELinuxMeter.c @@ -58,17 +58,10 @@ static bool isSelinuxEnforcing(void) { return false; } - int fd = open("/sys/fs/selinux/enforce", O_RDONLY); - if (fd < 0) { + char buf[20]; + 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) { -- cgit v1.2.3