On Mon, May 05, 2025 at 02:03:11PM -0400, Jeff King wrote: > On Mon, May 05, 2025 at 08:43:18AM -0700, Junio C Hamano wrote: > > > But for other kind of requirements, we want to fulfill them on all > > platforms that we claim to support. Using open_nofollow() to > > achieve hard atomicity requirement would be a bug in such a > > situation. Should we somehow warn our developers against its use? > > The comment above the declaration says: > > /* > * Open with O_NOFOLLOW, or equivalent. Note that the fallback equivalent > * may be racy. Do not use this as protection against an attacker who can > * simultaneously create paths. > */ > int open_nofollow(const char *path, int flags); > > though that may not be enough. 00611d8440 (add open_nofollow() helper, > 2021-02-16) discusses a way that it could be made less racy, at a > slightly increased cost. > > IMHO that is somewhat orthogonal to the issue here, though, which is > purely about the case where O_NOFOLLOW does exist (ironically, our > racy fallback code consistently returns ELOOP ;) ). > > The issue at hand is that particular errno responses are not always > portable. The patch discussed here improves that. My point was more that > I'm not sure to what degree we should care about errno consistency in > our wrappers (which is inherently a bit whack-a-mole as we find new > cases), versus trying not to care too hard about specific errno values > in calling code. > > I can see arguments either way (and as I said, an argument for making > errno values consistent even if we try to rely on them less). Mostly I > was just a little surprised to see open_nofollow() being used in this > way (especially since we have to end up stat()-ing anyway to check for > other cases). > IIRC, we wanted to try our best to make our code consistent. In the very early implementation, I actually firstly checked the file type and then opened the file. However, there is a chance that the raw "packed-refs" file could be converted to symlink between checking the filetype and opening the file to get the fd. Although, in fsck, we may just ignore this. But during the review, I found out that using "open_nofollow" could avoid race in some platforms. Sadly, I haven't realized that this would break compatibility ;) Because using "open_nofollow" could only check whether the filetype is the symlink, we also need to use "stat" again to check whether the filetype is OK. I agree that it is a little redundant. Since the patch from Collin would solve the problem. I won't change the logic. I'll focus on using `mmap` to open the "packed-refs" file. > -Peff Thanks, Jialuo
