fix #H6867 - mail buffer overrun

Web contact report of a github pull request.  A previous fix from
same user dealt with potential crash caused by freeing mailbox data
when the mailbox came from getenv("MAIL").  getenv() doesn't return
a value obtained by malloc so freeing it was bad.  The fix was to
allocate memory to hold a copy of getenv("MAIL") so that free() was
valid.  Unfortunately it didn't allocate enough space to hold the
terminating '\0' so potentially corrupted malloc/free bookkeeping
data.  And the alloc+copy was being performed every time the mailbox
was checked, resulting in leaked memory from the previous check (if
MAIL came from player's environment).  Fortunately the recheck only
takes place after new mail is actually detected and reported to the
player so the leak was probably small for most folks.

This compiles for the set of conditionals that apply to me (after
taking out -DNOMAIL that the hints put in my Makefile) but I can't
test that it actually works since mail is never delivered to this
machine.

diff --git a/doc/fixes36.1 b/doc/fixes36.1
index 9b8633956849395cf2c5854942f44e50711a8b62..0ce719fe45ccbc0dc12c05de79df04c1fad90f25 100644 (file)
--- a/doc/fixes36.1
+++ b/doc/fixes36.1
@@ -577,6 +577,11 @@ the fix for secret doors on special levels always having vertical orientation
 the fix intended for "a shop object stolen from outside the shop (via
        grappling hook) would be left marked as 'unpaid'" broke normal pickup,
        preventing any picked up item from merging with compatible stack
+unix: freeing mailbox data at game end crashed if MAIL came from environment
+unix: fix for freeing MAIL introduced a one-byte buffer overrun which could
+       interfere with malloc/free operation
+unix: fix for freeing MAIL also introduced a memory leak whenever new mail
+       is detected and MAIL comes from the environment
 
 
 Platform- and/or Interface-Specific Fixes

diff --git a/src/mail.c b/src/mail.c
index 9d8986db95e2252b383d23af723d5a1f003dcbaa..af5773af1b1a009a327aff3d6caf5eff8ad4425b 100644 (file)
--- a/src/mail.c
+++ b/src/mail.c
@@ -1,4 +1,4 @@
-/* NetHack 3.6 mail.c  $NHDT-Date: 1464222344 2016/05/26 00:25:44 $  $NHDT-Branch: NetHack-3.6.0 $:$NHDT-Revision: 1.27 $ */
+/* NetHack 3.6 mail.c  $NHDT-Date: 1519070343 2018/02/19 19:59:03 $  $NHDT-Branch: NetHack-3.6.0 $:$NHDT-Revision: 1.31 $ */
 /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */
 /* NetHack may be freely redistributed.  See license for details. */
 
@@ -90,38 +90,44 @@ free_maildata()
 void
 getmailstatus()
 {
-    char *emailbox;
-    if ((emailbox = nh_getenv("MAIL")) != 0) {
-        mailbox = (char *) alloc((unsigned) strlen(emailbox));
-        Strcpy(mailbox, emailbox);
-    }
-    if (!mailbox) {
+    if (mailbox) {
+        ; /* no need to repeat the setup */
+    } else if ((mailbox = nh_getenv("MAIL")) != 0) {
+        mailbox = dupstr(mailbox);
 #ifdef MAILPATH
+    } else  {
 #ifdef AMS
         struct passwd ppasswd;
 
-        (void) memcpy(&ppasswd, getpwuid(getuid()), sizeof(struct passwd));
+        (void) memcpy(&ppasswd, getpwuid(getuid()), sizeof (struct passwd));
         if (ppasswd.pw_dir) {
-            mailbox = (char *) alloc((unsigned) strlen(ppasswd.pw_dir)
-                                     + sizeof(AMS_MAILBOX));
+            /* note: 'sizeof "LITERAL"' includes +1 for terminating '\0' */
+            mailbox = (char *) alloc((unsigned) (strlen(ppasswd.pw_dir)
+                                                 + sizeof AMS_MAILBOX));
             Strcpy(mailbox, ppasswd.pw_dir);
             Strcat(mailbox, AMS_MAILBOX);
-        } else
-            return;
+        }
 #else
         const char *pw_name = getpwuid(getuid())->pw_name;
-        mailbox = (char *) alloc(sizeof(MAILPATH) + strlen(pw_name));
+
+        /* note: 'sizeof "LITERAL"' includes +1 for terminating '\0' */
+        mailbox = (char *) alloc((unsigned) (strlen(pw_name)
+                                             + sizeof MAILPATH));
         Strcpy(mailbox, MAILPATH);
         Strcat(mailbox, pw_name);
 #endif /* AMS */
-#else
-        return;
-#endif
+#endif /* MAILPATH */
     }
-    if (stat(mailbox, &omstat)) {
+
+    debugpline3("mailbox=%c%s%c",
+                mailbox ? '\"' : '<',
+                mailbox ? mailbox : "null",
+                mailbox ? '\"' : '>');
+
+    if (mailbox && stat(mailbox, &omstat)) {
 #ifdef PERMANENT_MAILBOX
         pline("Cannot get status of MAIL=\"%s\".", mailbox);
-        mailbox = 0;
+        free_maildata(); /* set 'mailbox' to Null */
 #else
         omstat.st_mtime = 0;
 #endif
@@ -495,7 +501,7 @@ ckmailstatus()
     if (stat(mailbox, &nmstat)) {
 #ifdef PERMANENT_MAILBOX
         pline("Cannot get status of MAIL=\"%s\" anymore.", mailbox);
-        mailbox = 0;
+        free_maildata();
 #else
         nmstat.st_mtime = 0;
 #endif