Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()

[Jeff King <peff@peff.net>]

On Wed, Apr 03, 2024 at 12:01:13PM +0200, René Scharfe wrote:

> > For mksnpath(), though, I
> > suspect that trying to format a very long name could result in the
> > output being full of uninitialized bytes.
> >
> > It only has one caller, which creates "foo~1" when we got EEXIST from
> > "foo". So I doubt you can get up to too much mischief with it. But it
> > could easily be replaced by mkpathdup() (or even a reusable strbuf
> > outside the loop if you really wanted to hyper-optimize)
> >
> > And then we could get rid of mksnpath() entirely, and its horrible
> > bad_path failure mode.
> 
> mkpath() would be perfect but unusable in multiple threads.  Cleaning
> up after mkpathdup() is a bit iffy in that caller.  strbuf would be a
> bit much in that error path, I think, and you might have to export or
> reimplement strbuf_cleanup_path().

Yeah, I'd prefer not to go to mkpath(), even though that's the simplest
thing, just because we should be reducing the subtle non-reentrant parts
of the code over time. I don't think the cleanup handling for
mkpathdup() is too bad:

diff --git a/apply.c b/apply.c
index 432837a674..15dfe607ff 100644
--- a/apply.c
+++ b/apply.c
@@ -4502,20 +4502,25 @@ static int create_one_file(struct apply_state *state,
 		unsigned int nr = getpid();
 
 		for (;;) {
-			char newpath[PATH_MAX];
-			mksnpath(newpath, sizeof(newpath), "%s~%u", path, nr);
+			char *newpath = mkpathdup("%s~%u", path, nr);
 			res = try_create_file(state, newpath, mode, buf, size);
-			if (res < 0)
+			if (res < 0) {
+				free(newpath);
 				return -1;
+			}
 			if (!res) {
-				if (!rename(newpath, path))
+				if (!rename(newpath, path)) {
+					free(newpath);
 					return 0;
+				}
 				unlink_or_warn(newpath);
+				free(newpath);
 				break;
 			}
 			if (errno != EEXIST)
 				break;
 			++nr;
+			free(newpath);
 		}
 	}
 	return error_errno(_("unable to write file '%s' mode %o"),

It even gets a little easier if you hoist it to a strbuf outside the
loop, as you don't need to free on each loop iteration. That seems worth
it to me to get rid of a function that IMHO is mis-designed (it is
really only useful if you assume paths are bounded by PATH_MAX, which we
know isn't always true).

> > diff --git a/usage.c b/usage.c
> > index 09f0ed509b..5baab98fa3 100644
> > --- a/usage.c
> > +++ b/usage.c
> > @@ -19,8 +19,10 @@ static void vreportf(const char *prefix, const char *err, va_list params)
> >  	}
> >  	memcpy(msg, prefix, prefix_len);
> >  	p = msg + prefix_len;
> > -	if (vsnprintf(p, pend - p, err, params) < 0)
> > +	if (vsnprintf(p, pend - p, err, params) < 0) {
> > +		if (snprintf(p, pend - p, "could not format error: %s", err) < 0)
> >  		*p = '\0'; /* vsnprintf() failed, clip at prefix */
> > +	}
> >
> >  	for (; p != pend - 1 && *p; p++) {
> >  		if (iscntrl(*p) && *p != '\t' && *p != '\n')
> >
> > Though most of the rest of the function is not that useful (it is mostly
> > removing unreadable chars, and hopefully we do not have any of those in
> > our format strings!).
> 
> For warnings and usage messages this would keep the prefix and not
> die.  This would look a bit strange, no?
> 
> 	usage: could not format error: TERRIBLE MESSAGE!

Sure, but I think any solution here is going to look strange. Keep in
mind that we're trying to improve the case where we print _nothing_
useful at all. If you do see this on a non-fatal message, the subsequent
messages may be informative. E.g.:

  error: could not format error: unable to open loose object %s
  fatal: bad object HEAD~12

is probably better than just exiting after the first.

> Yes, showing errno doesn't add that much value.  Except in this case it
> shows that there's something going on that I don't understand.  Dare I
> dig deeper?  Probably not..

Well, let us know if you do. ;)

-Peff