From: Stefan Sperling <stsp@stsp.name>
Subject: Re: reuse deltas while packing
To: "Todd C. Miller" <Todd.Miller@sudo.ws>
Cc: Christian Weisgerber <naddy@mips.inka.de>, gameoftrees@openbsd.org
Date: Mon, 7 Feb 2022 16:39:46 +0100

On Mon, Feb 07, 2022 at 08:26:38AM -0700, Todd C. Miller wrote:
> On Mon, 07 Feb 2022 15:32:10 +0100, Stefan Sperling wrote:
> 
> > This patch fixes got-index-pack getting stuck.
> >
> > Unfortunately, zlib uses an unsigned int to represent the number of
> > input bytes available in its input buffer, instead of a size_t.
> >
> > Large mapped files can cause truncation during our assignment from
> > size_t to unsigned int when we update zlib's input length counter.
> > This results in z->avail_in being set to zero, even though plenty
> > of bytes are available for zlib to consume. We then loop forever
> > asking zlib to decompress another zero bytes during every iteration.
> 
> Since the problem occurs when len - *consumed > UINT_MAX wouldn't
> something like this be a little better?
> 
> 	const size_t avail = len - *consumed;
> 	z->avail_in = MIN(avail, UINT_MAX);
> 
> That way the entire UINT_MAX is available.  Or if you prefer the
> if() style:
> 
> 	if (len - *consumed > UINT_MAX)
> 		z->avail_in = UINT_MAX;
> 	else
> 		z->avail_in = len - *consumed;
> 
> There should be no need to cast UINT_MAX to size_t since it is
> explicitly unsigned and should be implicitly promoted to size_t for
> the comparison.

Indeed, thank you. The code is much clearer this way.

This new patch still fixes the issue for me.

diff 0cc5a39811be39e9f2b47098ec3a38e8817ba0bc /home/stsp/src/got
blob - 6a151f9c54a8c0f4fc4fea9a4c91ecc7dec71aed
file + lib/deflate.c
--- lib/deflate.c
+++ lib/deflate.c
@@ -153,7 +153,10 @@ got_deflate_read_mmap(struct got_deflate_buf *zb, uint
 		size_t last_total_in = z->total_in;
 		if (z->avail_in == 0) {
 			z->next_in = map + offset + *consumed;
-			z->avail_in = len - *consumed;
+			if (len - *consumed > UINT_MAX)
+				z->avail_in = UINT_MAX;
+			else
+				z->avail_in = len - *consumed;
 			if (z->avail_in == 0) {
 				/* EOF */
 				ret = deflate(z, Z_FINISH);
blob - b0426141094227c79e5dd1d6e3cd0e5c2a0b28f0
file + lib/inflate.c
--- lib/inflate.c
+++ lib/inflate.c
@@ -249,7 +249,10 @@ got_inflate_read_mmap(struct got_inflate_buf *zb, uint
 				break;
 			}
 			z->next_in = map + offset + *consumed;
-			z->avail_in = len - *consumed;
+			if (len - *consumed > UINT_MAX)
+				z->avail_in = UINT_MAX;
+			else
+				z->avail_in = len - *consumed;
 		}
 		if (zb->csum) {
 			csum_in = z->next_in;
@@ -322,7 +325,6 @@ got_inflate_to_mem(uint8_t **outbuf, size_t *outlen,
 		if (zb.flags & GOT_INFLATE_F_HAVE_MORE) {
 			if (outbuf == NULL)
 				continue;
-			zb.outlen = (nbuf * GOT_INFLATE_BUFSIZE) - *outlen;
 			newbuf = reallocarray(*outbuf, ++nbuf,
 			    GOT_INFLATE_BUFSIZE);
 			if (newbuf == NULL) {
@@ -334,6 +336,7 @@ got_inflate_to_mem(uint8_t **outbuf, size_t *outlen,
 			}
 			*outbuf = newbuf;
 			zb.outbuf = newbuf + *outlen;
+			zb.outlen = (nbuf * GOT_INFLATE_BUFSIZE) - *outlen;
 		}
 	} while (zb.flags & GOT_INFLATE_F_HAVE_MORE);
 
@@ -382,7 +385,6 @@ got_inflate_to_mem_fd(uint8_t **outbuf, size_t *outlen
 		if (zb.flags & GOT_INFLATE_F_HAVE_MORE) {
 			if (outbuf == NULL)
 				continue;
-			zb.outlen = (nbuf * GOT_INFLATE_BUFSIZE) - *outlen;
 			newbuf = reallocarray(*outbuf, ++nbuf,
 			    GOT_INFLATE_BUFSIZE);
 			if (newbuf == NULL) {
@@ -394,6 +396,7 @@ got_inflate_to_mem_fd(uint8_t **outbuf, size_t *outlen
 			}
 			*outbuf = newbuf;
 			zb.outbuf = newbuf + *outlen;
+			zb.outlen = (nbuf * GOT_INFLATE_BUFSIZE) - *outlen;
 		}
 	} while (zb.flags & GOT_INFLATE_F_HAVE_MORE);