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

[shejialuo <shejialuo@gmail.com>]

On Sat, May 03, 2025 at 09:31:58AM -0400, Jeff King wrote:
> On Sat, May 03, 2025 at 12:57:11AM +0000, brian m. carlson wrote:
> 
> > > diff --git a/wrapper.c b/wrapper.c
> > > index 3c79778055..4d448d7c57 100644
> > > --- a/wrapper.c
> > > +++ b/wrapper.c
> > > @@ -737,7 +737,19 @@ int is_empty_or_missing_file(const char *filename)
> > >  int open_nofollow(const char *path, int flags)
> > >  {
> > >  #ifdef O_NOFOLLOW
> > > -	return open(path, flags | O_NOFOLLOW);
> > > +	int ret = open(path, flags | O_NOFOLLOW);
> > > +#ifdef __NetBSD__
> > > +	/*
> > > +	 * NetBSD sets errno to EFTYPE when path is a symlink. The only other
> > > +	 * time this errno occurs when O_REGULAR is used. Since we don't use
> > > +	 * it anywhere we can avoid an lstat here.
> > > +	 */
> > > +	if (ret < 0 && errno == EFTYPE) {
> > > +		errno = ELOOP;
> > > +		return -1;
> > > +	}
> > > +#endif
> > > +	return ret;
> > 
> > This patch seems reasonable and correct.  I don't use NetBSD, but I do
> > often test there, and I'm aware of this infelicity.  I'm surprised we
> > haven't hit it before.
> > 
> > I suspect we'll also hit this on FreeBSD, which has a similar issue in
> > that it returns `EMLINK` instead of `ELOOP`.  I do wish these two OSes
> > would provide an appropriate POSIX-compatible `open` call when set with
> > `_POSIX_SOURCE`, since this is one of the biggest portability problems
> > with them.
> 
> The inconsistency in errno has been there since open_nofollow() was
> added years ago. But we didn't notice it because in general we try not
> to be too particular about which errno value we receive.
> 
> That changed in cfea2f2da8 (packed-backend: check whether the
> "packed-refs" is regular file, 2025-02-28), which uses open_nofollow()
> to check for symlinks while we open it. But it feels like it would be
> more direct to just lstat() the file in the first place (which we end up
> doing anyway to check for other things besides symlinks!).
> 
> I.e., I'd think this would just work everywhere:
> 
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 3ad1ed0787..a247220df9 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -2071,35 +2071,32 @@ static int packed_fsck(struct ref_store *ref_store,
>  	if (o->verbose)
>  		fprintf_ln(stderr, "Checking packed-refs file %s", refs->path);
>  
> -	fd = open_nofollow(refs->path, O_RDONLY);
> -	if (fd < 0) {
> +	if (lstat(refs->path, &st) < 0) {
>  		/*
>  		 * If the packed-refs file doesn't exist, there's nothing
>  		 * to check.
>  		 */
>  		if (errno == ENOENT)
>  			goto cleanup;
>  
> -		if (errno == ELOOP) {
> -			struct fsck_ref_report report = { 0 };
> -			report.path = "packed-refs";
> -			ret = fsck_report_ref(o, &report,
> -					      FSCK_MSG_BAD_REF_FILETYPE,
> -					      "not a regular file but a symlink");
> -			goto cleanup;
> -		}
> -
> -		ret = error_errno(_("unable to open '%s'"), refs->path);
> -		goto cleanup;
> -	} else if (fstat(fd, &st) < 0) {
>  		ret = error_errno(_("unable to stat '%s'"), refs->path);
>  		goto cleanup;
> -	} else if (!S_ISREG(st.st_mode)) {
> +	}
> +
> +	if (!S_ISREG(st.st_mode)) {
>  		struct fsck_ref_report report = { 0 };
>  		report.path = "packed-refs";
>  		ret = fsck_report_ref(o, &report,
>  				      FSCK_MSG_BAD_REF_FILETYPE,
>  				      "not a regular file");
> +		/* XXX optionally could keep going here and actually
> +		 * check the contents we do find */
> +		goto cleanup;
> +	}
> +
> +	fd = open(refs->path, O_RDONLY);
> +	if (fd < 0) {
> +		ret = error_errno(_("unable to open '%s'"), refs->path);
>  		goto cleanup;
>  	}
>  
> diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
> index 9d1dc2144c..34d54a7c05 100755
> --- a/t/t0602-reffiles-fsck.sh
> +++ b/t/t0602-reffiles-fsck.sh
> @@ -632,7 +632,7 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '
>  		ln -sf packed-refs-back .git/packed-refs &&
>  		test_must_fail git refs verify 2>err &&
>  		cat >expect <<-EOF &&
> -		error: packed-refs: badRefFiletype: not a regular file but a symlink
> +		error: packed-refs: badRefFiletype: not a regular file
>  		EOF
>  		rm .git/packed-refs &&
>  		test_cmp expect err &&
> 
> It's not as "atomic" as open_nofollow() and fstat(), but I don't think
> we care about that for fsck. This is about consistency checking, not
> trying to beat races against active adversaries (not to mention that our
> open_nofollow() is best-effort anyway, and may be racy).
> 

The motivation why I use "open_nofollow" is that we want to avoid race
conditions as many as possible. However, as you have said, our
"open_nofollow" function still has the race. And it would be a little
cumbersome to make "open_nofollow" compatible. So, I rather prefer that
we simply use this way to solve the problem.

> I dunno. I don't mind making errno returns more consistent to prevent a
> future foot-gun, but I think as a general rule we may be better off not
> looking too hard at errno for exotic conditions.
> 

I don't mind either.

> -Peff
> 
> PS I notice that this same function reads the whole packed-refs file
>    into a strbuf. That may be a problem, as they can grow pretty big in
>    extreme cases (e.g., GitHub's fork networks easily got into the
>    gigabytes, as it was every ref of every fork). We usually mmap it.
>    Not related to this discussion, but just something I noticed while
>    reading the function.

Peff, thanks for notifying me. I want to know more background.
Initially, the reason why I don't use `mmap` is that when checking the
ref consistency, we usually don't need to share the "packed-refs"
content for multiple processes via `mmap`.

I don't know how Github executes "git fsck" for the forked repositories.
Is there any regular tasks for "git fsck"? And would "packed-refs" file
be shared for all these repositories?

If above is the case, I agree that we should reuse the logic of
"load_contents" to enhance. But I don't know whether we need to do this
in the first place.

Thanks,
Jialuo