Re: [PATCH 16/21] compat/fsmonitor: remove dependency on `the_repository` in Darwin IPC

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]

On Tue, Jun 11, 2024 at 11:16:03PM +0000, brian m. carlson wrote:
> On 2024-06-11 at 11:58:47, Patrick Steinhardt wrote:
> > The IPC socket used by the fsmonitor on Darwin is usually contained in
> > the Git repository itself. When the repository is hosted on a networked
> > filesystem though, we instead create the socket path in the user's home
> > directory or the socket directory. In that case, we derive the path by
> > hashing the repository path.
> > 
> > The hashing implicitly depends on `the_repository` though via
> > `hash_to_hex()`. For one, this is wrong because we really should be
> > using the passed-in repository. But arguably, it is not sensible to
> > derive the path hash from the repository's object hash in the first
> > place -- they don't have anything to do with each other, and a
> > repository that is hosted in the same path should always derive the same
> > IPC socket path.
> > 
> > Fix this by unconditionally using SHA1 to derive the path.
> 
> Let's instead use SHA-256 to derive the path.  I can imagine that there
> might be a time when some users would like to drop support for SHA-1
> altogether (for FIPS compliance reasons, say) and we'll make our lives a
> lot easier if we avoid more uses of SHA-1.
> 
> It is also typically faster when compiled appropriately, although the
> amount of data we're processing is very small.

I only now realize that this actually a bug. The hash is already getting
computed as SHA1 unconditionally like this:

	git_SHA1_Init(&sha1ctx);
	git_SHA1_Update(&sha1ctx, r->worktree, strlen(r->worktree));
	git_SHA1_Final(hash, &sha1ctx);

And then we used to pass the computed hash to `hash_to_hex()`, which is
of course the wrong thing to do in a SHA256 repository because we would
end up printing `GIT_MAX_RAWSZ - GIT_SHA1_RAWSZ` uninitialized bytes.

I agree that we want to convert this to SHA256 eventually. But I'd say
we should keep such a backwards-incompatible change out of this patch
series and handle it as a follow-up.

I'll rephrase the commit message though.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]