Re: [Bug] Git ReadOnly Temp Packfile Causes "Bad file descriptor" And -13 Access Error With NFSv4

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Feb 11, 2025, 22:46 UTC
Message-ID: <Z6vTNW9Dz4jiJvXO@tapette.crustytoothpaste.net>
In-Reply-To: <20250211213940.GB3113114@coredump.intra.peff.net>

On 2025-02-11 at 21:39:40, Jeff King wrote:

Show 8 quoted lines

> We could probably create and write the tempfile as 0644, and then switch
> it to 0444 before renaming it into place. We already do something
> similar with adjust_shared_perm(), I think.
> 
> I don't know if it's worth doing it that way or not. It should just work
> even on systems that are happy with the current code, so I think the
> only downside would be a few extra lines of code and an extra chmod()
> syscall.

That may work here for this instance. My concern is that we'll find some other weird, hard-to-reproduce behaviour because files are silently getting reopened and permissions are getting re-checked. This bug is much worse than the standard NFS/CIFS implementation bug where open with O_RDWR and 0444 just fails up front, because it can happen in the middle of an operation we thought was going to be successful.

I'm not completely opposed to a small workaround here (although, as I mentioned, a decent amount of other software would also need patching for such a system, which argues against it), but I don't want us to have lots of workarounds scattered all over the code to patch one flaky set of NFS servers, so we'd want to limit it to this one spot, especially since this won't be easy to test.

I will also mention that this NFS server will also break lots of proprietary software. One of my former employers had software that would do lots of open/fchmod in quick succession (for good and valuable reasons that are too long to fit in this parenthetical), and I'm pretty sure that won't work properly on this server based on the described behaviour.

-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA