Re: [PATCH] wrapper: Fix a errno discrepancy on NetBSD.

[shejialuo <shejialuo@gmail.com>]

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