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