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

Alejandro Colomar via Mutt-dev <[email protected]> Thu, 7 May 2026 14:09:23 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<afx_CG9S32DrGZHC@devuan>
Hi Derek,

On 2026-05-07T13:58:36+0200, Alejandro Colomar wrote:
> 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.

Thus, the diff becomes:

	diff --git i/muttlib.c w/muttlib.c
	index 59a48378..f8d6b32e 100644
	--- i/muttlib.c
	+++ w/muttlib.c
	@@ -1387,13 +1387,39 @@ 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 || strrspn_(dir, "/"))
	     fmt = "%s%s";
	 
	+  fname = stpspn(fname, "/");
	+
	   mutt_buffer_printf(d, fmt, dir, fname);
	 }

Actually, this conflates a readability improvement with strrspn_(), and
a bug fix with stpspn().  I would break this into two separate patches,
of course.  (And then the one for the NULL pointer.)

So, the real bug fix is only

	diff --git i/muttlib.c w/muttlib.c
	index 59a48378..399efd39 100644
	--- i/muttlib.c
	+++ w/muttlib.c
	@@ -1387,6 +1387,14 @@ void mutt_safe_path(BUFFER *dest, ADDRESS *a)
	       *p = '_';
	 }
	 
	+// 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";
	@@ -1394,6 +1402,8 @@ void mutt_buffer_concat_path(BUFFER *d, const char *dir, const char *fname)
	   if (!*fname || (*dir && dir[strlen(dir)-1] == '/'))
	     fmt = "%s%s";
	 
	+  fname = stpspn(fname, "/");
	+
	   mutt_buffer_printf(d, fmt, dir, fname);
	 }

And if for the stable version you want to avoid adding a new API, you
can reduce it further to just:

	diff --git i/muttlib.c w/muttlib.c
	index 59a48378..a9fe8779 100644
	--- i/muttlib.c
	+++ w/muttlib.c
	@@ -1394,6 +1394,8 @@ void mutt_buffer_concat_path(BUFFER *d, const char *dir, const char *fname)
	   if (!*fname || (*dir && dir[strlen(dir)-1] == '/'))
	     fmt = "%s%s";
	 
	+  fname += strspn(fname, "/");
	+
	   mutt_buffer_printf(d, fmt, dir, fname);
	 }
	 
I would recommend applying this minimal fix to stable, but in master,
I'd add stpspn(), which prevents easy and dangerous bugs which were
discussed in the list recently.


Have a lovely day!
Alex

> 
> > 	+  }
> > 	+
> > 	   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>



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