Harden common file helpers against null dereferences, fd 0 mishandling, and relative-path crashes

src/common/utils/file.c

@@ -129,22 +129,41 @@ char *file_read(const char *path)
         length = ftell(f);
         fseek(f, 0, SEEK_SET);
         buffer = (char *)malloc((length + 1) * sizeof(char));
-        if (buffer)
-            fread(buffer, sizeof(char), length, f);
+        if (!buffer) {
+            fclose(f);
+            return NULL;
+        }
+
+        fread(buffer, sizeof(char), length, f);
         fclose(f);
     }
+    else {
+        return NULL;
+    }
+
     buffer[length] = '\0';
 
     return buffer;
 }
 
 bool file_write(const char *path, const char *str, uint32_t len)
 {
-    uint32_t fd;
-    if ((fd = open(path, O_WRONLY)) == 0)
+    int fd;
+    ssize_t written;
+
+    if ((fd = open(path, O_WRONLY)) == -1)
         return false;
-    if (write(fd, str, len) == -1)
+
+    written = write(fd, str, len);
+    if (written == -1) {
+        close(fd);
         return false;
+    }
+    if ((size_t)written != len) {
+        close(fd);
+        return false;
+    }
+
     close(fd);
     return true;
 }
@@ -467,8 +486,13 @@ void file_add_line_to_beginning(const char *filename, const char *lineToAdd)
     }
     char tempPath[STR_MAX];
     char *path = file_dirname(filename);
-    sprintf(tempPath, "%s/temp.txt", path);
+    int written = snprintf(tempPath, sizeof(tempPath), "%s%stemp.txt", path ? path : "", path ? "/" : "");
     free(path);
+    if (written < 0 || (size_t)written >= sizeof(tempPath)) {
+        fclose(file);
+        print_debug("Temporary file path is too long");
+        return;
+    }
 
     FILE *tempFile = fopen(tempPath, "w");
     if (tempFile == NULL) {

src/common/utils/str.c

@@ -94,27 +94,28 @@ size_t str_trim(char *out, size_t len, const char *str, bool first)
     bool is_string = false;
 
     // Trim leading space
-    while (strchr("\r\n\t {},", (unsigned char)*str) != NULL)
+    while (*str != 0 && strchr("\r\n\t {},", (unsigned char)*str) != NULL)
         str++;
 
-    end = str + 1;
+    if (*str == 0) // All spaces?
+    {
+        *out = 0;
+        return 0;
+    }
 
     if ((unsigned char)*str == '"') {
         is_string = true;
         str++;
-        while (strchr("\r\n\"", (unsigned char)*end) == NULL)
+        end = str;
+        while (*end != 0 && strchr("\r\n\"", (unsigned char)*end) == NULL)
             end++;
     }
-
-    if (*str == 0) // All spaces?
-    {
-        *out = 0;
-        return 1;
-    }
+    else
+        end = str;
 
     // Trim trailing space
     if (first)
-        while (strchr("\r\n\t {},", (unsigned char)*end) == NULL)
+        while (*end != 0 && strchr("\r\n\t {},", (unsigned char)*end) == NULL)
             end++;
     else {
         end = str + strlen(str) - 1;
@@ -167,7 +168,8 @@ void str_removeParentheses(char *str_out, const char *str_in)
                 inside = false;
             continue;
         }
-        temp[c++] = str_in[i];
+        if (c < STR_MAX - 1)
+            temp[c++] = str_in[i];
     }
 
     temp[c] = '\0';
@@ -177,6 +179,10 @@ void str_removeParentheses(char *str_out, const char *str_in)
 
 void str_serializeTime(char *dest_str, int nTime)
 {
+    if (nTime < 0) {
+        nTime = 0;
+    }
+
     if (nTime >= 60) {
         int h = nTime / 3600;
         int m = (nTime - 3600 * h) / 60;
@@ -195,7 +201,8 @@ void str_serializeTime(char *dest_str, int nTime)
 int str_count_char(const char *str, char ch)
 {
     int i, count = 0;
-    for (i = 0; i <= strlen(str); i++) {
+    int len = strlen(str);
+    for (i = 0; i < len; i++) {
         if (str[i] == ch) {
             count++;
         }
@@ -214,4 +221,4 @@ bool includeCJK(char *str)
         str++;
     }
     return false;
-}
+}

test/Makefile

@@ -6,5 +6,14 @@ include ../src/common/config.mk
 TARGET = test
 LDFLAGS := $(LDFLAGS) -L../lib -s -lSDL_image -lSDL -lSDL_rotozoom -lgtest -lgtest_main -lpthread
 
+STR_TEST_TARGET = str_test
+STR_TEST_OFILES = test_str.o ../src/common/utils/str.o ../src/common/utils/file.o
+
 include ../src/common/commands.mk
-include ../src/common/recipes.mk
+include ../src/common/recipes.mk
+
+$(STR_TEST_TARGET): $(STR_TEST_OFILES)
+	@mkdir -p "$(BUILD_DIR)"
+	@$(CC) $(STR_TEST_OFILES) -o "$(BUILD_DIR)/$(STR_TEST_TARGET)"
+
+str-test: $(STR_TEST_TARGET)

test/test_str.c

@@ -0,0 +1,96 @@
+#include <assert.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include "../src/common/utils/file.h"
+#include "../src/common/utils/str.h"
+
+int main(void)
+{
+    char out[STR_MAX];
+    char hidden_suffix[] = {'a', '\0', 'b', '\0'};
+    char long_name[STR_MAX + 32];
+    size_t trimmed;
+
+    assert(str_count_char("", 'a') == 0);
+    assert(str_count_char("abc", 'a') == 1);
+    assert(str_count_char("a/b/c", '/') == 2);
+    assert(str_count_char("abc", '\0') == 0);
+    assert(str_count_char(hidden_suffix, 'b') == 0);
+
+    trimmed = str_trim(out, sizeof(out), "", false);
+    assert(trimmed == 0);
+    assert(strcmp(out, "") == 0);
+
+    trimmed = str_trim(out, sizeof(out), "   ", false);
+    assert(trimmed == 0);
+    assert(strcmp(out, "") == 0);
+
+    trimmed = str_trim(out, sizeof(out), "  abc  ", false);
+    assert(trimmed == 3);
+    assert(strcmp(out, "abc") == 0);
+
+    trimmed = str_trim(out, sizeof(out), "\"abc\"", false);
+    assert(trimmed == 3);
+    assert(strcmp(out, "abc") == 0);
+
+    str_removeParentheses(out, "Game Title (USA) [Hack]");
+    assert(strcmp(out, "Game Title") == 0);
+
+    memset(long_name, 'x', sizeof(long_name) - 1);
+    long_name[sizeof(long_name) - 1] = '\0';
+    str_removeParentheses(out, long_name);
+    assert(strlen(out) == STR_MAX - 2);
+    for (size_t i = 0; i < strlen(out); i++)
+        assert(out[i] == 'x');
+
+    str_serializeTime(out, -1);
+    assert(strcmp(out, "0s") == 0);
+
+    str_serializeTime(out, 0);
+    assert(strcmp(out, "0s") == 0);
+
+    str_serializeTime(out, 59);
+    assert(strcmp(out, "59s") == 0);
+
+    str_serializeTime(out, 60);
+    assert(strcmp(out, "1m 0s") == 0);
+
+    str_serializeTime(out, 3600);
+    assert(strcmp(out, "1h 0m") == 0);
+
+    char temp_path[] = "/tmp/onion-file-test-XXXXXX";
+    int temp_fd = mkstemp(temp_path);
+    assert(temp_fd >= 0);
+    close(temp_fd);
+
+    int saved_stdin = dup(STDIN_FILENO);
+    assert(saved_stdin >= 0);
+    assert(close(STDIN_FILENO) == 0);
+    int write_ok = file_write(temp_path, "abc", 3);
+    assert(dup2(saved_stdin, STDIN_FILENO) == STDIN_FILENO);
+    close(saved_stdin);
+    assert(write_ok);
+
+    char *file_contents = file_read(temp_path);
+    assert(file_contents != NULL);
+    assert(strcmp(file_contents, "abc") == 0);
+    free(file_contents);
+    unlink(temp_path);
+
+    char relative_path[] = "onion-relative-XXXXXX";
+    int relative_fd = mkstemp(relative_path);
+    assert(relative_fd >= 0);
+    assert(write(relative_fd, "world\n", 6) == 6);
+    close(relative_fd);
+
+    file_add_line_to_beginning(relative_path, "hello\n");
+    file_contents = file_read(relative_path);
+    assert(file_contents != NULL);
+    assert(strcmp(file_contents, "hello\nworld\n") == 0);
+    free(file_contents);
+    unlink(relative_path);
+
+    return 0;
+}