From: Stefan Sperling <stsp@stsp.name>
Subject: Re: deltify addblk() fixes
To: Christian Weisgerber <naddy@mips.inka.de>
Cc: gameoftrees@openbsd.org
Date: Sun, 13 Jun 2021 16:45:36 +0200

On Fri, Jun 11, 2021 at 09:49:18PM +0200, Christian Weisgerber wrote:
> Stefan Sperling:
> 
> > > There's no point in reading a block until we know that we need to
> > > compare it.  The "if (r == 0)" part should move under the following
> > > if.
> > 
> > Yes, that makes sense. This patch implemenents your suggestion.
> 
> >  		if (len == dt->blocks[i].len && h == dt->blocks[i].hash) {
> > +			uint8_t buf[GOT_DELTIFY_MAXCHUNK];
> 
> I think that is wrong.  The lifetime of an automatic object is the
> _block_ where it is declared.

You are right. I wanted to declare both buffers together since they
are used as a pair. Let's move buf2 to function scope instead, then.

ok like this?

diff 68bdcdc2f5d3c37d918f85368c2537a8aa7d90eb /home/stsp/src/got
blob - 45add0094489e3d46e675165b1d5379f56a01a4a
file + lib/deltify.c
--- lib/deltify.c
+++ lib/deltify.c
@@ -101,6 +101,7 @@ addblk(struct got_delta_table *dt, FILE *f, off_t len,
 	const struct got_error *err = NULL;
 	int i;
 	uint8_t buf[GOT_DELTIFY_MAXCHUNK];
+	uint8_t buf2[GOT_DELTIFY_MAXCHUNK];
 	size_t r = 0;
 
 	if (len == 0)
@@ -108,23 +109,22 @@ addblk(struct got_delta_table *dt, FILE *f, off_t len,
 
 	i = h % dt->nalloc;
 	while (dt->blocks[i].len != 0) {
-		if (r == 0) {
-			if (fseeko(f, offset, SEEK_SET) == -1)
-				return got_error_from_errno("fseeko");
-			r = fread(buf, 1, len, f);
-			if (r != len) {
-				if (ferror(f))
-					return got_ferror(f, GOT_ERR_IO);
-				return NULL;
-			}
-		}
 		/*
 		 * Avoid adding duplicate blocks.
 		 * NB: A matching hash is insufficient for detecting equality.
 		 * The hash can only detect inequality.
 		 */
 		if (len == dt->blocks[i].len && h == dt->blocks[i].hash) {
-			uint8_t buf2[GOT_DELTIFY_MAXCHUNK];
+			if (r == 0) {
+				if (fseeko(f, offset, SEEK_SET) == -1)
+					return got_error_from_errno("fseeko");
+				r = fread(buf, 1, len, f);
+				if (r != len) {
+					if (!ferror(f))
+						return NULL;
+					return got_ferror(f, GOT_ERR_IO);
+				}
+			}
 			if (fseeko(f, dt->blocks[i].offset, SEEK_SET) == -1)
 				return got_error_from_errno("fseeko");
 			if (fread(buf2, 1, len, f) != len)