mutt_buffer_concat_path is broken [PATCH: 1/1]

So, I came upon this by random happenstance, but this is a problem
I've had to fix numerous times in production code, because seemingly
absolutely no one realizes that concatenating paths correctly is
substantially more complicated to get right than you'd initially
expect.

I wrote a little test program to demonstrate the issue.  The current
implementation can produce paths with double slashes (which,
admittedly, is mostly harmless on most/all POSIX systems, unless
you're going to display them), and can also crash if either dir or
fname are NULL (which is NOT harmless, and which it does not properly
check for, though there's an attempt to handle the fname case).

The test results of my version vs. the current:

$ ./mutt
Path 1     Path 2     Expected Result      Derek's version      Status  Old Version         Status 
--------------------------------------------------------------------------------------------------
"foo/bar"  "baz"      foo/bar/baz          "foo/bar/baz"        PASS   "foo/bar/baz"        PASS    
"foo/bar/" "baz"      foo/bar/baz          "foo/bar/baz"        PASS   "foo/bar/baz"        PASS    
"foo/bar/" "/baz"     foo/bar/baz          "foo/bar/baz"        PASS   "foo/bar//baz"       FAIL    
""         "/baz"     /baz                 "/baz"               PASS   "//baz"              FAIL    
"/"        "foo"      /foo                 "/foo"               PASS   "/foo"               PASS    
"/"        "/foo"     /foo                 "/foo"               PASS   "//foo"              FAIL    
"foo/bar"  ""         foo/bar              "foo/bar"            PASS   "foo/bar"            PASS    
"foo/bar"  "(null)"   foo/bar              "foo/bar"            PASS   Segmentation fault (core dumped)

And the second run, necessitated by the crash, and my unwillingness to
write signal handler code to continue the test (which might not even
work).  The last case swaps which input is NULL from the run above,
albeit with the same end result:

$ ./mutt
Path 1     Path 2     Expected Result      Derek's Version      Status  Old Version         Status 
--------------------------------------------------------------------------------------------------
"foo/bar"  "baz"      foo/bar/baz          "foo/bar/baz"        PASS   "foo/bar/baz"        PASS    
"foo/bar/" "baz"      foo/bar/baz          "foo/bar/baz"        PASS   "foo/bar/baz"        PASS    
"foo/bar/" "/baz"     foo/bar/baz          "foo/bar/baz"        PASS   "foo/bar//baz"       FAIL    
""         "/baz"     /baz                 "/baz"               PASS   "//baz"              FAIL    
"/"        "foo"      /foo                 "/foo"               PASS   "/foo"               PASS    
"/"        "/foo"     /foo                 "/foo"               PASS   "//foo"              FAIL    
"foo/bar"  ""         foo/bar              "foo/bar"            PASS   "foo/bar"            PASS    
"(null)"   "baz"      baz                  "baz"                PASS   Segmentation fault (core dumped)

Note:  I did not attempt to handle the silly case of the caller
passing in args with multiple leading/trailing slashes, though that
could be done easily enough.  I figure that case is extremely unlikely
and it's already complex enough.  Also note that this version obviates
the need for at least some checking done in the code prior to calling
this function.

I'm happy to provide the test code as well, upon request (though
clearly it will not work if you just apply the patch--you have to keep
the original version and add mine as mutt_buffer_concat_path_new).

Lastly, it's not obvious that NULL arguments shouldn't produce some
sort of error, or how to handle that.  I return NULL if the args are
too broken, and the buffer address otherwise; it might be more
appropriate to abort or some such.

-- 
Derek D. Martin    http://www.pizzashack.org/   GPG Key ID: 0xDFBEAD02
-=-=-=-=-
This message is posted from an invalid address.  Replying to it will result in
undeliverable mail due to spam prevention.  Sorry for the inconvenience.

diff --git a/muttlib.c b/muttlib.c
index 59a48378..5a58d0ec 100644
--- a/muttlib.c
+++ b/muttlib.c
@@ -1387,14 +1387,48 @@ void mutt_safe_path(BUFFER *dest, ADDRESS *a)
       *p = '_';
 }
 
-void mutt_buffer_concat_path(BUFFER *d, const char *dir, const char *fname)
-{
-  const char *fmt = "%s/%s";
-
-  if (!*fname || (*dir && dir[strlen(dir)-1] == '/'))
-    fmt = "%s%s";
-
-  mutt_buffer_printf(d, fmt, dir, fname);
+BUFFER *mutt_buffer_concat_path_new(BUFFER *d, const char *dir, const char *fname)
+{
+  /* arguments are broken; return NULL */
+  if (!d || (!dir && !fname)) return NULL;
+  if (dir && *dir){
+    if (strcmp(dir, "/") != 0){
+      /* dir IS NOT the root directory */
+      mutt_buffer_addstr(d, dir);
+      if (fname && *fname){
+        if (fname[0] == '/' && dir[strlen(dir)-1] == '/'){
+          /* avoid double slash when dir is the root directory and file name starts with a slash */
+          mutt_buffer_addstr(d, &fname[1]);
+        } else {
+          if (fname[0] != '/' && dir[strlen(dir)-1] != '/'){
+            /* avoid double slash when dir is the root directory */
+            mutt_buffer_addch(d, '/');
+          }
+          /* append the file name */
+          mutt_buffer_addstr(d, fname);
+        }
+      }
+    } else {
+      /* dir IS the root directory */
+      if (fname && *fname){
+        if (fname[0] == '/'){
+          /* file name already starts with a slash, just append it */
+          mutt_buffer_addstr(d, fname);
+        } else {
+            /* file name doesn't start with '/' */
+            mutt_buffer_addch(d, '/');
+            mutt_buffer_addstr(d, fname);
+        }
+      } else {
+        /* dir is the root directory and fname is empty, add a slash */
+        mutt_buffer_addch(d, '/');
+      }
+    }
+  } else {
+    /* dir is empty, just append the file name */
+      mutt_buffer_addstr(d, fname);
+  }
+  return d;
 }
 
 /*