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

"Kevin J. McCarthy" <[email protected]> Thu, 7 May 2026 11:03:43 +0800
Newsgroups	gmane.mail.mutt.devel
Message-ID	<afwBD9R-hrxv9h6K@qinghai>
On Wed, May 06, 2026 at 03:56:34PM -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.

Hi Derek,

Thanks for taking the time to write a patch!

First some general comments.

Please don't change the return type.
Mutt's general policy is to not pass NULL strings around, instead it 
wraps parameters that might be NULL inside the NONULL() macro.  See, for 
example, the invocations in init.c:3839,3841.  Similarly, if the caller 
passes a NULL "BUFFER *d" argument, let the program crash: that's just 
completely fubar.

So, deal with !*dir and !*fname, but don't check for dir and fname (or 
d) being NULL.

Also please match the formatting style used in Mutt, i.e. Allman curly 
brace style, being free to omit the braces for single lines if you like.

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);
+BUFFER *mutt_buffer_concat_path_new(BUFFER *d, const char *dir, const char *fname)

As commented about, leave this as void return type.

+{
+  /* arguments are broken; return NULL */
+  if (!d || (!dir && !fname)) return NULL;

And don't check this.

Previously, the code implicitly cleared the BUFFER (inside 
mutt_buffer_printf().  So you should call mutt_buffer_clear(d) before 
anything else.

+  if (dir && *dir){

I'll stop repeating this now, but all checks for NULL dir and fname 
should be taken out here and below.

+    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 */

I think this comment was what you were talking about in your reply to
yourself, so make sure to fix it.  If you don't think it's too verbose,
it might be great to see examples along with the comment.  e.g.:
           /* avoid double slash: "/dir/" + "/fname/foo.txt" */


+          mutt_buffer_addstr(d, &fname[1]);
+        } else {
+          if (fname[0] != '/' && dir[strlen(dir)-1] != '/'){
+            /* avoid double slash when dir is the root directory */

            /* add missing slash: "/dir" + "fname/foo.txt" */

+            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 */

           /* "/" + "/fname/foo.txt" */

+          mutt_buffer_addstr(d, fname);
+        } else {

          /* "/" + "fname/foo.txt" */

+            /* 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;
  }


-- 
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C  5308 ADEF 7684 8031 6BDA
signature.asc (application/pgp-signature, 833 B)