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
