Re: mutt_buffer_concat_path is broken [PATCH: 1/1]

Alejandro Colomar via Mutt-dev <[email protected]> Wed, 6 May 2026 23:15:19 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<afuph0GETNeeu6f1@devuan>
Hi Derek,

On 2026-05-06T15:56:34-0400, Derek Martin wrote:
> 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);

To be honest, I find the code below completely unreadable.

I propose a much simpler fix, which needs adding a few simple interfaces
(I've put them nearby, just for writing it quickly):

	diff --git i/muttlib.c w/muttlib.c
	index 59a48378..c4b51c75 100644
	--- i/muttlib.c
	+++ w/muttlib.c
	@@ -1387,13 +1387,46 @@ void mutt_safe_path(BUFFER *dest, ADDRESS *a)
	       *p = '_';
	 }
	 
	+// strnul - string null-byte
	+#define strnul(s)  strchr(s, '\0')
	+
	+// strrspn_ - string rear span
	+size_t
	+strrspn_(const char *s, const char *accept)
	+{
	+       size_t      spn;
	+       const char  *p;
	+
	+       p = strnul(s);
	+       for (spn = 0; p > s; spn++) {
	+               p--;
	+               if (strchr(accept, *p) == NULL)
	+                       return spn;
	+       }
	+       return spn;
	+}
	+
	+// stpspn - string returns-pointer span
	+#define stpspn(s, accept)                                    \
	+({                                                           \
	+       __auto_type  s_ = s;                                  \
	+                                                             \
	+       s_ + strspn(s_, accept);                              \
	+})
	+
	 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] == '/'))
	+  if (!*fname)
	     fmt = "%s%s";
	 
	+  if (strrspn(dir, "/"))
	+  {
	+    fmt = "%s%s";
	+    fname = stpspn(fname, "/");
	+  }
	+
	   mutt_buffer_printf(d, fmt, dir, fname);
	 }

I haven't tested (not even compiled) this code.  It may need a few small
tweaks.

The fix above doesn't fix the segfault, but I think that deserves
a separate commit that just adds another small branch.


Have a lovely night!
Alex

> +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;
>  }
>  
>  /*


-- 
<https://www.alejandro-colomar.es>
signature.asc (application/pgp-signature, 833 B)