Re: [PATCH 01/11] path: harden validation of HEAD with non-standard hashes

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The `validate_headref()` function takes a path to a supposed "HEAD" file
> > and checks whether its format is something that we understand. It is
> > used as part of our repository discovery to check whether a specific
> > directory is a Git directory or not.
> >
> > Part of the validation is a check for a detached HEAD that contains a
> > plain object ID. To do this validation we use `get_oid_hex()`, which
> > relies on `the_hash_algo`. At this point in time the hash algo cannot
> > yet be initialized though because we didn't yet read the Git config.
> > Consequently, it will always be the SHA1 hash algorithm.
> >
> > In practice this works alright because `get_oid_hex()` only ends up
> > checking whether the prefix of the buffer is a valid object ID. And
> > because SHA1 is shorter than SHA256, the function will successfully
> > parse SHA256 object IDs, as well.
> >
> > It is somewhat fragile though and not really the intent to only check
> > for SHA1. With this in mind, harden the code to use `get_oid_hex_any()`
> > to check whether the "HEAD" file parses as any known hash.
> 
> All makes sense, and given the above, I strongly suspect that we
> would want to make the validate_headref() function a file-scope
> static in setup.c to make sure it is only called/callable from the
> repository discovery codepath.  Especially that if somebody calls
> this function again after we find out that the repository uses
> SHA-256, we would want to let the caller know that the detached HEAD
> records SHA-1 and we are in an inconsistent state.

> > It follows that we could just leave the current code intact, as in
> > practice the code change doesn't have any user visible impact. But it
> > also prepares us for `the_hash_algo` being unset when there is no
> > repositroy.
> 
> Or perhaps we use get_oid_hex_any() != GIT_HASH_UNKNOWN when
> the_hash_algo has not been determined, and use !get_oid_hex() after
> we have determined which algorithm we are using?  It may be what you
> did in a later step in the series, so let me read on.

> Thanks.
> 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  path.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/path.c b/path.c
> > index 67229edb9c..cc02165530 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -693,7 +693,7 @@ int validate_headref(const char *path)
> >  	/*
> >  	 * Is this a detached HEAD?
> >  	 */
> > -	if (!get_oid_hex(buffer, &oid))
> > +	if (get_oid_hex_any(buffer, &oid) != GIT_HASH_UNKNOWN)
> >  		return 0;
> >  
> >  	return -1;