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