On Wed, Apr 03, 2024 at 04:48:36PM -0400, Jeff King wrote:
> 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"),
OK, this misses one of the breaks, and potentially clobbers errno in
that path by calling free(). So it is harder than I said. I still think
it's worth doing, though.
-Peff
