From: Stefan Sperling <stsp@stsp.name>
Subject: Re: fix dubious checksum failures in got-fetch-pack
To: "Todd C. Miller" <Todd.Miller@millert.dev>
Cc: gameoftrees@openbsd.org
Date: Sun, 30 Oct 2022 08:19:15 +0100

On Sat, Oct 29, 2022 at 03:35:42PM -0600, Todd C. Miller wrote:
> On Sat, 29 Oct 2022 23:27:34 +0200, Stefan Sperling wrote:
> 
> > Using a work-in-progress regression test suite for gotd, I can trigger
> > occasional checksum failures in got-fetch-pack while cloning a repository.
> >
> > This is a long-standing bug in got-fetch-pack, where only one byte stored
> > in the potential sha1 checksum buffer is moved (copied) up, instead of
> > shifting all of the remaining bytes up by one position, as was intended. 
> > As a result, the checksum we end up computing for the pack file is incorrect.
> >
> > This can be triggered if the server is doing very short writes, and is
> > probably rare enough with git-daemon/Github that we haven't seen it fail
> > very often.
> 
> Your fix looks correct, I'm just not sure why you need to call
> SHA1Update() and memmove() in a loop like this.  Can't you just
> call SHA1Update() once?
> 
>  - todd
> 

Like this? Still works in my testing.

(Yes, I know MIN should be avoided outside kernel code. Moving this over
to MINIMUM in the entire got.git tree is left for later).

diff refs/heads/main refs/heads/fetch-csum
commit - f5d30fbb425287166f3a3ce74eca6adf547bdef7
commit + 83f24e77c74421cc5141bf4e4c9e87d0b05cefe7
blob - b253e5ac03dea40d28e1f2112418a29f0365a7c6
blob + 6cac7e61e2407f2c053f4cf8256c67ae466489a6
--- libexec/got-fetch-pack/got-fetch-pack.c
+++ libexec/got-fetch-pack/got-fetch-pack.c
@@ -52,6 +52,10 @@
 #include "got_lib_gitproto.h"
 #include "got_lib_ratelimit.h"
 
+#ifndef MIN
+#define	MIN(_a,_b) ((_a) < (_b) ? (_a) : (_b))
+#endif
+
 #ifndef nitems
 #define nitems(_a)	(sizeof((_a)) / sizeof((_a)[0]))
 #endif
@@ -685,11 +689,14 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 				 * amount of bytes out at the front to make
 				 * room, mixing those bytes into the checksum.
 				 */
-				while (sha1_buf_len > 0 &&
+				if (sha1_buf_len > 0 &&
 				    sha1_buf_len + r > SHA1_DIGEST_LENGTH) {
-					SHA1Update(&sha1_ctx, sha1_buf, 1);
-					memmove(sha1_buf, sha1_buf + 1, 1);
-					sha1_buf_len--;
+					size_t nshift = MIN(sha1_buf_len + r -
+					    SHA1_DIGEST_LENGTH, sha1_buf_len);
+					SHA1Update(&sha1_ctx, sha1_buf, nshift);
+					memmove(sha1_buf, sha1_buf + nshift,
+					    sha1_buf_len - nshift);
+					sha1_buf_len -= nshift;
 				}
 
 				/* Buffer potential checksum bytes. */