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

Alejandro Colomar via Mutt-dev <[email protected]> Thu, 7 May 2026 13:58:33 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<afx-Rvkv5bBjoGqd@devuan>
On 2026-05-06T23:15:19+0200, Alejandro Colomar via Mutt-dev wrote:
> 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, "/");

Actually, the line above should be out of the conditional.  We should
never keep the leading slash of 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>



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