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

["René Scharfe" <l.s.r@web.de>]

Am 05.04.24 um 00:53 schrieb Jeff King:
> 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).

Horrible.  OS implementation details peeking through an API that has no
return value and no defined errors is outright Lovecraftian.

> 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.

Yes.

> 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?).

I think performing this ritual automatically every time on all platforms
(i.e. to use git_free() unconditionally) to provide sane errno values
around free(3) calls is better than having to worry about attacks from
the deep.  But then again I'm easily scared and still a bit in shock, so
perhaps I'm overreacting.

René