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