From: Omar Polo <op@omarpolo.com>
Subject: Re: got-read-patch: don't hardcode SHA1_DIGEST_LENGTH
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Fri, 02 Aug 2024 00:49:35 +0200

On 2024/08/01 22:08:53 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Thu, Aug 01, 2024 at 05:40:12PM +0200, Omar Polo wrote:
> > This is mostly cosmetic.  it doesn't cause any practical harm on sha256
> > repos.  Since we hardcode the sha1 digest length (which should be
> > _STRING too...) we just truncate the object id in some cases, which in
> > turns means we have to search for a matching object.
> > 
> > Since we don't know in this libexec the object format, and this is just
> > an hint anyway[*], let's drop the pedantic check.
> > 
> > ok?
> 
> Not sure if we should simply delete this check. It does seem useful,
> even if just to avoid sending an invalid object ID to callers of
> this function.
> 
> What about trying to parse a sha2 hash first, and a sha1 hash if
> sha2 failed to parse, and only fail the entire check if both of
> these parsing attempts fail?

I'm attaching a diff that does that.

> > [*]: i'm considering to change how the merge strategy works so that if
> >      we fail to patch the provided blob, we retry using the local file
> >      instead.  This will also fix some strange error messages like the
> >      one Lucas hit by manually editing a patch.
> 
> Sure, seems unrelated to this parsing sanity check though.

more or less.  By deleting the check I was making the routine slightly
more prone to pick up garbage that could in turn be by chance a valid
blob id prefix.  however, you're right that's a separate discussion.

anyway, here's a diff that does what you're suggesting.

diff /home/op/w/got
commit - 13d14aacc10fc9281e7598041152351f23c1592c
path + /home/op/w/got
blob - 9b5c6b818d19c41eb3e9e4ffede13cd4a6fb81f5
file + libexec/got-read-patch/got-read-patch.c
--- libexec/got-read-patch/got-read-patch.c
+++ libexec/got-read-patch/got-read-patch.c
@@ -184,7 +184,7 @@ filexbit(const char *line)
 static const struct got_error *
 blobid(const char *line, char **blob, int git)
 {
-	uint8_t digest[SHA1_DIGEST_LENGTH];
+	uint8_t digest[GOT_HASH_DIGEST_STRING_MAXLEN];
 	size_t len;
 
 	*blob = NULL;
@@ -193,7 +193,11 @@ blobid(const char *line, char **blob, int git)
 	if ((*blob = strndup(line, len)) == NULL)
 		return got_error_from_errno("strndup");
 
-	if (!git && !got_parse_hash_digest(digest, *blob, GOT_HASH_SHA1)) {
+	if (git)
+		return NULL;
+
+	if (!got_parse_hash_digest(digest, *blob, GOT_HASH_SHA1) &&
+	    !got_parse_hash_digest(digest, *blob, GOT_HASH_SHA256)) {
 		/* silently ignore invalid blob ids */
 		free(*blob);
 		*blob = NULL;