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

[Jeff King <peff@peff.net>]

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

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.

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