[Nmh-workers] Re: Diffs for replacing mktemp() usage

[Earl Hood]

On February 2, 2010 at 13:40, Earl Hood wrote:

> Attached are the changes I have currently done in my development
> environment.  The changes also include minor mods to deal
> with "may be uninitialized" warnings from gcc.

Found a problem with mhparse.c with the changes I did.  Temporary files
can be left around when show mime messages.  The reason is the code
takes the temp pathname and appends a suffix to it based on mime type
(if a suffix is defined), then it uses that pathname, causing the
existing ("waiting") temp file to be left.

I made changes to mhparse.c so if a suffix is to be appended,
it will rename the existing, waiting temporary file vs creating
a new file (which exposes nmh to the race problem).

Included is the diff of mhparse.c against CVS HEAD.

Index: uip/mhparse.c
===================================================================
RCS file: /cvsroot/nmh/nmh/uip/mhparse.c,v
retrieving revision 1.21
diff -u -r1.21 mhparse.c
--- uip/mhparse.c       14 Aug 2008 06:19:08 -0000      1.21
+++ uip/mhparse.c       3 Feb 2010 05:19:42 -0000
@@ -204,12 +204,14 @@
      * Check if file is actually standard input
      */
     if ((is_stdin = !(strcmp (file, "-")))) {
-       file = add (m_tmpfil (invo_name), NULL);
-       if ((fp = fopen (file, "w+")) == NULL) {
-           advise (file, "unable to fopen for writing and reading");
-           return NULL;
-       }
+        char *tfile = m_mktemp2(NULL, invo_name, NULL, &fp);
+        if (tfile == NULL) {
+            advise("mhparse", "unable to create temporary file");
+            return NULL;
+        }
+       file = add (tfile, NULL);
        chmod (file, 0600);
+
        while (fgets (buffer, sizeof(buffer), stdin))
            fputs (buffer, fp);
        fflush (fp);
@@ -1764,7 +1766,7 @@
     }
 
     if (*file == NULL) {
-       ce->ce_file = add (m_scratch ("", tmp), NULL);
+       ce->ce_file = add (m_mktemp(tmp, NULL, NULL), NULL);
        ce->ce_unlink = 1;
     } else {
        ce->ce_file = add (*file, NULL);
@@ -1781,8 +1783,21 @@
                   ci->ci_type);
         cp = context_find (buffer);
     }
-    if (cp != NULL && *cp != '\0')
-        ce->ce_file = add (cp, ce->ce_file);
+    if (cp != NULL && *cp != '\0') {
+        if (ce->ce_unlink) {
+            // Temporary file already exists, so we rename to
+            // version with extension.
+            char *file_org = strdup(ce->ce_file);
+            ce->ce_file = add (cp, ce->ce_file);
+            if (rename(file_org, ce->ce_file)) {
+                adios (ce->ce_file, "unable to rename %s to ", file_org);
+            }
+            free(file_org);
+
+        } else {
+            ce->ce_file = add (cp, ce->ce_file);
+        }
+    }
 
     if ((ce->ce_fp = fopen (ce->ce_file, "w+")) == NULL) {
        content_error (ce->ce_file, ct, "unable to fopen for reading/writing");
@@ -1972,7 +1987,7 @@
     }
 
     if (*file == NULL) {
-       ce->ce_file = add (m_scratch ("", tmp), NULL);
+       ce->ce_file = add (m_mktemp(tmp, NULL, NULL), NULL);
        ce->ce_unlink = 1;
     } else {
        ce->ce_file = add (*file, NULL);
@@ -1989,8 +2004,21 @@
                   ci->ci_type);
         cp = context_find (buffer);
     }
-    if (cp != NULL && *cp != '\0')
-        ce->ce_file = add (cp, ce->ce_file);
+    if (cp != NULL && *cp != '\0') {
+        if (ce->ce_unlink) {
+            // Temporary file already exists, so we rename to
+            // version with extension.
+            char *file_org = strdup(ce->ce_file);
+            ce->ce_file = add (cp, ce->ce_file);
+            if (rename(file_org, ce->ce_file)) {
+                adios (ce->ce_file, "unable to rename %s to ", file_org);
+            }
+            free(file_org);
+
+        } else {
+            ce->ce_file = add (cp, ce->ce_file);
+        }
+    }
 
     if ((ce->ce_fp = fopen (ce->ce_file, "w+")) == NULL) {
        content_error (ce->ce_file, ct, "unable to fopen for reading/writing");
@@ -2177,7 +2205,7 @@
     }
 
     if (*file == NULL) {
-       ce->ce_file = add (m_scratch ("", tmp), NULL);
+       ce->ce_file = add (m_mktemp(tmp, NULL, NULL), NULL);
        ce->ce_unlink = 1;
     } else {
        ce->ce_file = add (*file, NULL);
@@ -2194,8 +2222,21 @@
                   ci->ci_type);
         cp = context_find (buffer);
     }
-    if (cp != NULL && *cp != '\0')
-        ce->ce_file = add (cp, ce->ce_file);
+    if (cp != NULL && *cp != '\0') {
+        if (ce->ce_unlink) {
+            // Temporary file already exists, so we rename to
+            // version with extension.
+            char *file_org = strdup(ce->ce_file);
+            ce->ce_file = add (cp, ce->ce_file);
+            if (rename(file_org, ce->ce_file)) {
+                adios (ce->ce_file, "unable to rename %s to ", file_org);
+            }
+            free(file_org);
+
+        } else {
+            ce->ce_file = add (cp, ce->ce_file);
+        }
+    }
 
     if ((ce->ce_fp = fopen (ce->ce_file, "w+")) == NULL) {
        content_error (ce->ce_file, ct, "unable to fopen for reading/writing");
@@ -2545,7 +2586,7 @@
     else if (caching)
        ce->ce_file = add (cachefile, NULL);
     else
-       ce->ce_file = add (m_scratch ("", tmp), NULL);
+       ce->ce_file = add (m_mktemp(tmp, NULL, NULL), NULL);
 
     if ((ce->ce_fp = fopen (ce->ce_file, "w+")) == NULL) {
        content_error (ce->ce_file, ct, "unable to fopen for reading/writing");
@@ -2747,7 +2788,7 @@
     }
 
     if (*file == NULL) {
-       ce->ce_file = add (m_scratch ("", tmp), NULL);
+       ce->ce_file = add (m_mktemp(tmp, NULL, NULL), NULL);
        ce->ce_unlink = 1;
     } else {
        ce->ce_file = add (*file, NULL);