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
Description: PGP signature
