Merge pull request #74 from NathanNeurotic/codex/fix-codacy-issues-in-specified-files

Harden debug logging and fix unsafe/partial reads

Author: NathanNeurotic

include/debugprintf.h

@@ -52,6 +52,12 @@ static inline void debugprintf(const char *fmt, ...)
 #define NO_DPRINTF
 #endif
 
-#define DPRINTF(...) debugprintf(__VA_ARGS__)
+#ifdef NO_DPRINTF
+#define DPRINTF(...) ((void)0)
+#define DPRINTS(s)   ((void)0)
+#else
+#define DPRINTF(fmt, ...) debugprintf("" fmt "", ##__VA_ARGS__)
+#define DPRINTS(s)        debugprintf("%s", (s))
+#endif
 
 #endif //DEBUG_PRINTF

mk_kelf.sh

@@ -46,5 +46,9 @@ cp LICENSE kelf/LICENSE.TXT
 cp README.md kelf/README.md
 mv kelf/ $TARGET
 7z a -t7z PS2BBL_KELF.7z "$TARGET/*"
+if [ "$TARGET" = "/" ] || [ "$TARGET" = "." ]; then
+  echo "Refusing to remove unsafe TARGET: '$TARGET'" >&2
+  exit 1
+fi
 rm -rf -- "$TARGET/"
 echo "done!"

src/OSDHistory.c

@@ -42,6 +42,21 @@ static int HasTooManyHistoryRecords;
 static struct HistoryEntry OldHistoryEntry;
 struct HistoryEntry HistoryEntries[MAX_HISTORY_ENTRIES];
 
+static int read_full(int fd, void *buf, size_t nbytes)
+{
+    unsigned char *p = buf;
+    size_t got = 0;
+
+    while (got < nbytes) {
+        ssize_t r = read(fd, p + got, nbytes - got);
+        if (r <= 0)
+            return -1;
+        got += (size_t)r;
+    }
+
+    return 0;
+}
+
 #ifdef F_WriteHistoryFile
 static int WriteHistoryFile(int port, const char *path, const void *buffer, int len, int append)
 {
@@ -130,14 +145,7 @@ int LoadHistoryFile(int port)
     result = 0;
     if (fd >= 0) {
         size_t expected = MAX_HISTORY_ENTRIES * sizeof(struct HistoryEntry);
-        size_t total = 0;
-        while (total < expected) {
-            ssize_t chunk = read(fd, (u8 *)HistoryEntries + total, expected - total);
-            if (chunk <= 0)
-                break;
-            total += (size_t)chunk;
-        }
-        if (total != expected)
+        if (read_full(fd, HistoryEntries, expected) < 0)
             result = -EIO;
 
         close(fd);

src/OSDInit.c

@@ -562,10 +562,10 @@ int OSDInitROMVER(void)
     memset(ConsoleROMVER, 0, ROMVER_MAX_LEN);
     if ((fd = open("rom0:ROMVER", O_RDONLY)) >= 0) {
         ssize_t len = read(fd, ConsoleROMVER, ROMVER_MAX_LEN - 1);
-        if (len > 0 && len < ROMVER_MAX_LEN)
-            ConsoleROMVER[len] = '\0';
-        else
+        if (len < 0)
             ConsoleROMVER[0] = '\0';
+        else
+            ConsoleROMVER[len] = '\0';
         close(fd);
     }
 

src/dvdplayer.c

@@ -308,11 +308,11 @@ int DVDPlayerInit(void)
         return 0;
     }
 
-    int bytes = read(fd, id, sizeof(id) - 1);
-    if (bytes > 0 && bytes < (int)sizeof(id))
-        id[bytes] = '\0';
-    else
+    ssize_t bytes = read(fd, id, sizeof(id) - 1);
+    if (bytes < 0)
         id[0] = '\0';
+    else
+        id[bytes] = '\0';
     close(fd);
 
     ROMDVDPlayer.major = atoi(id);
@@ -340,11 +340,11 @@ int DVDPlayerInit(void)
             fd = open("rom1:DVDVER", O_RDONLY);
 
         if (fd >= 0) {
-            result = read(fd, ROMDVDPlayer.ver, sizeof(ROMDVDPlayer.ver) - 1);
-            if (result > 0 && result < (int)sizeof(ROMDVDPlayer.ver))
-                ROMDVDPlayer.ver[result] = '\0';
-            else
+            ssize_t version_len = read(fd, ROMDVDPlayer.ver, sizeof(ROMDVDPlayer.ver) - 1);
+            if (version_len < 0)
                 ROMDVDPlayer.ver[0] = '\0';
+            else
+                ROMDVDPlayer.ver[version_len] = '\0';
             close(fd);
         }
     }

src/main.c

@@ -519,10 +519,10 @@ int main(int argc, char *argv[])
 
     if ((fd = open("rom0:ROMVER", O_RDONLY)) >= 0) {
         ssize_t len = read(fd, ROMVER, sizeof(ROMVER) - 1);
-        if (len > 0 && len < (ssize_t)sizeof(ROMVER))
-            ROMVER[len] = '\0';
-        else
+        if (len < 0)
             ROMVER[0] = '\0';
+        else
+            ROMVER[len] = '\0';
         close(fd);
     }
     j = SifLoadModule("rom0:ADDDRV", 0, NULL); // Load ADDDRV. The OSD has it listed in rom0:OSDCNF/IOPBTCONF, but it is otherwise not loaded automatically.
@@ -1028,22 +1028,13 @@ static int __attribute__((unused)) LocateExternalIRXPath(const char *filename, c
 
     for (i = 0; i < (sizeof(search_templates) / sizeof(search_templates[0])); i++) {
         const char *tmpl = search_templates[i];
-        const char *placeholder = strstr(tmpl, "%s");
-        if (placeholder == NULL)
+        if (strstr(tmpl, "%s") == NULL)
             continue;
 
-        size_t prefix_len = (size_t)(placeholder - tmpl);
-        size_t suffix_len = strlen(placeholder + 2);
-        size_t filename_len = strlen(filename);
-        size_t required = prefix_len + filename_len + suffix_len + 1;
-        if (required > resolved_size)
+        int ret = util_snprintf(resolved_path, resolved_size, tmpl, filename);
+        if (ret < 0 || (size_t)ret >= resolved_size)
             continue;
 
-        memcpy(resolved_path, tmpl, prefix_len);
-        memcpy(resolved_path + prefix_len, filename, filename_len);
-        memcpy(resolved_path + prefix_len + filename_len, placeholder + 2, suffix_len);
-        resolved_path[required - 1] = '\0';
-
         const char *checked_path = CheckPath(resolved_path);
         if (checked_path != NULL && exist(checked_path)) {
             return 0;