Fix security and stability issues from code review

docs/CHANGELOG.md

@@ -5,6 +5,38 @@ All notable changes to restoHack will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [1.1.7] - 2026-06-05
+
+### Fixed
+
+- **SECURITY**: Format string vulnerabilities in inventory display
+  - `pline(xprname(...))` in `hack.invent.c` — user-named objects containing `%` became format strings
+  - `pline(buf)` in `hack.options.c` — option state buffer passed directly as format string
+- **SECURITY**: `free()` on arena-allocated `toptenentry` in `prscore()` (hack.end.c)
+  - `newttentry()` returns pointers into a static arena; the old `free()` was undefined behavior
+  - Removed; `reset_topten_arena()` at the top of each caller handles cleanup
+- **SECURITY**: `getenv("HOME")` NULL dereference in `child()` (hack.pager.c)
+  - Crashes in stripped environments (containers, setuid, cron) where HOME is unset
+  - Now falls back to `/tmp` with a warning to stderr
+- **SECURITY**: Missing `setuid(getuid())` privilege drop before `exec` in `child()`
+  - Only `setgid` was being dropped; a comment at the call site already claimed `setuid` was done
+  - Added `setuid(getuid())` after `setgid(getgid())` so the pager exec runs unprivileged
+- **STABILITY**: Out-of-bounds read in `dowhatis()` (hack.pager.c)
+  - `ep[-1]` when `ep == NULL` (no newline found by `index()`) caused a null pointer deref
+  - Guard is now `ep && ep > buf && ep[-1] == ';'`
+- **STABILITY**: Three file descriptor leaks in `dorecover()` on early-return paths (hack.save.c)
+  - Struct size mismatch, unexpected version, and pre-versioned format rejection paths all leaked fd
+- **STABILITY**: `check_save_header()` used panic-backed `mread()` for post-magic header fields
+  - Truncated save files triggered `panic("dungeon collapses")` instead of a graceful rejection
+  - Replaced `sr_u16`/`sr_u32` calls with a single `read()` that returns 0 on short read; removed now-dead `sr_u16`
+- **STABILITY**: `save_pos` typed `long` instead of `off_t` in `dorecover()` (hack.save.c)
+  - `lseek()` returns `off_t`; assignment to `long` truncates on ILP32 large-file platforms
+  - Initial `lseek` now also checked for failure
+- **STABILITY**: lseek I/O failure and "too old" save format were indistinguishable (hack.save.c)
+  - Seek failure now branches to its own error path with a distinct message
+- **CORRECTNESS**: `resize_pending` signal flag typed `volatile int` instead of `volatile sig_atomic_t`
+  - Written from SIGWINCH handler; `sig_atomic_t` is required by the C standard for signal safety
+
 ## [1.1.6] 2026-03-15
 
 ### Fixed

src/hack.end.c

@@ -837,7 +837,7 @@ void prscore(int argc, char **argv) {
           break;
         }
       }
-    free((char *)t1);
+    /* Modern: Removed free() - t1 is arena-allocated, reset_topten_arena() handles cleanup */
   }
 #ifdef nonsense
   totchars[totcharct] = 0;

src/hack.invent.c

@@ -572,7 +572,7 @@ int max;
     if (ckfn && !(*ckfn)(otmp))
       continue;
     if (!allflag) {
-      pline(xprname(otmp, ilet));
+      pline("%s", xprname(otmp, ilet)); /* Modern: Fix format string vulnerability */
       addtopl(" [nyaq]? ");
       sym = readchar();
     } else
@@ -615,7 +615,7 @@ struct obj *obj;
 
 void prinv(obj) struct obj *obj;
 {
-  pline(xprname(obj, obj_to_let(obj)));
+  pline("%s", xprname(obj, obj_to_let(obj))); /* Modern: Fix format string vulnerability */
 }
 
 static char *xprname(struct obj *obj, char let) /* ANSI C conversion from K&R style */

src/hack.main.c

@@ -62,7 +62,7 @@ void hangup(int sig);
  * dynamic terminal sizing in modern window managers. Maintains game state
  * integrity during resize events.
  */
-static volatile int resize_pending = 0;
+static volatile sig_atomic_t resize_pending = 0; /* Modern: sig_atomic_t required for signal handler writes */
 static int original_CO = 0, original_LI = 0;
 
 void handle_resize(int sig);

src/hack.options.c

@@ -307,7 +307,7 @@ int doset(void) {
         buf[len - 1] = 0;
       }
     }
-    pline(buf);
+    pline("%s", buf); /* Modern: Fix format string vulnerability */
   } else
     parseoptions(buf, FALSE);
 

src/hack.pager.c

@@ -80,7 +80,7 @@ int dowhatis(void) {
             (void)strncpy(buf + 1, "       ", 7);
           }
           pline("%s", buf); /* MODERN: Fix format string vulnerability */
-          if (ep[-1] == ';') {
+          if (ep && ep > buf && ep[-1] == ';') { /* Modern: ep can be NULL; ep>buf guards UB if ep==buf */
             pline("More info? ");
             if (readchar() == 'y') {
               page_more(fp, 1); /* does fclose() */
@@ -448,6 +448,9 @@ int child(int wt) {
       /* Privilege dropping failed, but continue - it's advisory */
       perror("setgid warning");
     }
+    /* Modern: Drop UID before exec - comment at caller claimed this was already done */
+    if (setuid(getuid()) != 0)
+      perror("setuid warning");
 
 #ifdef CHDIR
 #if 0
@@ -456,9 +459,14 @@ int child(int wt) {
 #endif
     /**
      * MODERN ADDITION (2025): Improved chdir error handling*/
-    if (chdir(getenv("HOME")) != 0) {
-      /* Failed to change to HOME directory, warn but continue */
-      perror("chdir to HOME warning");
+    {
+      const char *home = getenv("HOME"); /* Modern: NULL-safe; HOME unset in containers/setuid */
+      if (!home) {
+        fprintf(stderr, "warning: HOME not set, using /tmp as working directory\n");
+        home = "/tmp";
+      }
+      if (chdir(home) != 0)
+        perror("chdir to HOME warning");
     }
 #endif /* CHDIR */
     return (1);

src/hack.save.c

@@ -59,11 +59,6 @@ static void sw_u16(int fd, rh_u16_t val) {
   bwrite(fd, (char *)buf, 2);
 }
 
-static rh_u16_t sr_u16(int fd) {
-  unsigned char buf[2];
-  mread(fd, (char *)buf, 2);
-  return ((rh_u16_t)buf[0] << 8) | buf[1];
-}
 
 static void sw_u32(int fd, rh_u32_t val) {
   unsigned char buf[4];
@@ -229,9 +224,18 @@ static int check_save_header(int fd, rh_hdr_t *hdr) {
   /* Copy magic to header */
   memcpy(hdr->magic, test_magic, 4);
 
-  hdr->version = sr_u16(fd);
-  hdr->endiantag = sr_u32(fd);
-  hdr->reserved = sr_u32(fd);
+  /* Modern: Use direct read() not sr_u16/sr_u32 — mread() panics on short read,
+   * but this function is supposed to return 0 gracefully on truncated files. */
+  {
+    unsigned char rem[10];
+    if (read(fd, rem, 10) != 10)
+      return 0;
+    hdr->version    = ((rh_u16_t)rem[0] << 8) | rem[1];
+    hdr->endiantag  = ((rh_u32_t)rem[2] << 24) | ((rh_u32_t)rem[3] << 16)
+                    | ((rh_u32_t)rem[4] << 8)  |  rem[5];
+    hdr->reserved   = ((rh_u32_t)rem[6] << 24) | ((rh_u32_t)rem[7] << 16)
+                    | ((rh_u32_t)rem[8] << 8)  |  rem[9];
+  }
 
   /* Check endianness */
   if (hdr->endiantag != RH_ENDIANTAG) {
@@ -505,7 +509,13 @@ int dorecover(int fd) {
 
   /* Check if this is a versioned save file */
   rh_hdr_t hdr;
-  long save_pos = lseek(fd, 0, SEEK_CUR); /* Save current position */
+  off_t save_pos = lseek(fd, 0, SEEK_CUR); /* Modern: off_t not long; lseek returns off_t */
+  if (save_pos == (off_t)-1) {
+    perror("lseek: cannot determine save file position");
+    (void)close(fd);
+    restoring = FALSE;
+    return (0);
+  }
 
   if (check_save_header(fd, &hdr)) {
     /* Versioned format detected */
@@ -530,6 +540,7 @@ int dorecover(int fd) {
     if (you_size != sizeof(struct you)) {
       puts("Save file struct size mismatch.");
       restoring = FALSE;
+      (void)close(fd); /* Modern: Fix fd leak on early return */
       return (0);
     }
     sr_bytes(fd, &u, sizeof(struct you));
@@ -597,6 +608,7 @@ int dorecover(int fd) {
       /* Unknown version (shouldn't happen due to check_save_header) */
       impossible("Unexpected save version", hdr.version, 0);
       restoring = FALSE;
+      (void)close(fd); /* Modern: Fix fd leak on early return */
       return (0);
     }
 
@@ -616,9 +628,16 @@ int dorecover(int fd) {
      *
      * For canonical hosting safety, these are intentionally rejected.
      */
-    lseek(fd, save_pos, SEEK_SET);
+    if (lseek(fd, save_pos, SEEK_SET) == (off_t)-1) { /* Modern: Branch I/O errors separately */
+      perror("lseek on save file");
+      puts("I/O error reading save file. Cannot restore.");
+      restoring = FALSE;
+      (void)close(fd);
+      return (0);
+    }
     puts("Save file is too old (pre-Version 1). Cannot load safely.");
     restoring = FALSE;
+    (void)close(fd);
     return (0);
   }
   restnames(fd);