| Message ID | d4e87f9d6b4813fe359e22f4b5d5ebda28e09a08.1718106285.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce `USE_THE_REPOSITORY_VARIABLE` macro | expand |
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.
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
diff --git a/compat/fsmonitor/fsm-ipc-darwin.c b/compat/fsmonitor/fsm-ipc-darwin.c index 6f3a95410c..b4d21d6dc2 100644 --- a/compat/fsmonitor/fsm-ipc-darwin.c +++ b/compat/fsmonitor/fsm-ipc-darwin.c @@ -41,9 +41,10 @@ const char *fsmonitor_ipc__get_path(struct repository *r) /* Create the socket file in either socketDir or $HOME */ if (sock_dir && *sock_dir) { strbuf_addf(&ipc_file, "%s/.git-fsmonitor-%s", - sock_dir, hash_to_hex(hash)); + sock_dir, hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1])); } else { - strbuf_addf(&ipc_file, "~/.git-fsmonitor-%s", hash_to_hex(hash)); + strbuf_addf(&ipc_file, "~/.git-fsmonitor-%s", + hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1])); } free(sock_dir);
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. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- compat/fsmonitor/fsm-ipc-darwin.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)