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@xxxxxxxx> >> Signed-off-by: René Scharfe <l.s.r@xxxxxx> > > 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é
