free and errno, was Re: [PATCH] apply: replace mksnpath() with a mkpathdup() call

[Jeff King <peff@peff.net>]

On Thu, Apr 04, 2024 at 11:08:38PM +0200, René Scharfe wrote:

> Support paths longer than PATH_MAX in create_one_file() (which is not a
> hard limit e.g. on Linux) by calling mkpathdup() instead of mksnpath().
> Remove the latter, as it becomes unused by this change.  Resist the
> temptation of using the more convenient mkpath() to avoid introducing a
> dependency on a static variable deep inside the apply machinery.
> 
> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: René Scharfe <l.s.r@web.de>

Heh, so obviously I had the same patch stewing. But one thing that gave
me pause is: do we need to worry about preserving errno across free()
boundaries?

Traditionally touching errno was something free() was allowed to do, and
there were definitely cases where glibc would do so (mostly due to
munmap). But recent versions of POSIX clarify that it should not touch
errno, and glibc as of 2.33 does not (which dates to 2021).

This issue from 2015 talks about "the next version of POSIX" making that
change:

  https://sourceware.org/bugzilla/show_bug.cgi?id=17924

but it looks to me from:

  https://www.austingroupbugs.net/view.php?id=385

that the change wasn't accepted there until 2020 (and AFAICT that hasn't
resulted in a new published spec yet).

Now it would be pretty darn useful to not have to worry about preserving
errno. It's subtle code that's hard to remember is needed, and sometimes
hard to get right depending on the rest of the flow control.

Years like "2020" and "2021" are too recent for us to say "oh, that's
ancient history, we don't have to care anymore". But I wonder if we can
be a little cavalier here for two reasons:

  - it's rare; for the most part free() isn't going to touch errno.
    Failures from munmap() are pretty rare, and small allocations like
    this are probably done with sbrk() anyway. Of course that's just
    talking about glibc, and there are other platforms. But it still
    seems like it should be a rarity for any free() implementation to
    fail or to want to touch errno.

  - the stakes are usually pretty low; the outcome in most cases would
    be a misleading error message as we clobber errno. But note that it
    does sometimes affect control flow (this patch is an example; we are
    checking for EEXIST to break out of the loop).

So would it be that unreasonable to assume the modern, desired behavior,
and mostly shrug our shoulders for other platforms? We could even
provide:

  #ifdef FREE_CLOBBERS_ERRNO
  void git_free(void *ptr)
  {
        int saved_errno = errno;
        free(ptr);
        errno = saved_errno;
  }
  #define free(ptr) git_free(ptr)
  #endif

if we really wanted to be careful, though it's hard to know which
platforms even need it (and again, it's so rare to come up in practice
that I'd suspect people could go for a long time before realizing their
platform was a problem). I guess the flip side is that we could use the
function above by default, and disable it selectively (the advantage of
disabling it being that it's slightly more efficient; maybe that's not
even measurable?).

>  		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;
>  			}
> +			free(newpath);
>  			if (errno != EEXIST)
>  				break;
>  			++nr;

At any rate, you can probably see the places where free() clobbering
errno would be a problem here. Our return when "res < 0" (though I don't
think any of the callers actually care about errno after that), the
check for EEXIST at the bottom of the loop, and after we break out of
the loop, we use error_errno() to report it.

-Peff