"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
fix off-by-one causing invalid deltas
To:
gameoftrees@openbsd.org
Date:
Mon, 21 Jul 2025 18:37:41 +0200

Download raw body.

Thread 
I found a seperate bug while trying to write a test for the problem
reported by Aaron.

When deltification attempts to stretch a common block of data to the
maximum possible size, we have an off-by-one which can cause the block
to be larger than the maximum block size which can be represented in
an encoded delta. The result is an invalid delta which does not copy
enough data and gets flagged by both gotadmin indexpack and git index-pack.

The new regression test included with this patch triggers the problem:

got-index-pack: delta application result size mismatch: \
        actual: 65536 expected: 16777216: bad delta

16777216 in the error above equals (1 << 24)

The problem is on this line, where we effectively need - 2 instead of - 1:

	while (buf_equal && *blocklen < (1 << 24) - 1) {

Also, the inner loop which runs (*blocklen)++ without a bounds check
could end up moving *blocklen way past the limit.

I am including the diff I already sent earlier with Subject
  "delta.c overrides errors in some cases"
because diagnostic changes I made depend on this other diff.
I would commit them separately.

ok?

M  lib/delta.c                                  |  10+  7-
M  lib/deltify.c                                |   8+  2-
M  lib/got_lib_deltify.h                        |   1+  0-
M  regress/gotd/Makefile                        |  11+  1-
A  regress/gotd/large_loose_objects.sh          |  63+  0-
A  regress/gotd/prepare_large_loose_objects.sh  |  33+  0-

6 files changed, 126 insertions(+), 10 deletions(-)

commit - ae784b42e06f525e360bf478bb382845f718fc09
commit + 5776bdd665dca1c0e328b8937a400deccadf8017
blob - 2c3f8c2ebcd41b76c90f5701a0b1ffc43fbdccc3
blob + 111b5f80283a779cf10f175b7652acea1cd5fccf
--- lib/delta.c
+++ lib/delta.c
@@ -320,9 +320,11 @@ got_delta_apply_in_mem(uint8_t *base_buf, size_t base_
 		}
 	}
 
-	if (*outsize != result_size)
-		err = got_error_msg(GOT_ERR_BAD_DELTA,
-		    "delta application result size mismatch");
+	if (err == NULL && *outsize != result_size)
+		err = got_error_fmt(GOT_ERR_BAD_DELTA,
+		    "delta application result size mismatch: actual: %zd "
+		    "expected: %llu", *outsize, result_size);
+
 	return err;
 }
 
@@ -387,12 +389,13 @@ got_delta_apply(FILE *base_file, const uint8_t *delta_
 		}
 	}
 
-	if (*outsize != result_size)
-		err = got_error_msg(GOT_ERR_BAD_DELTA,
-		    "delta application result size mismatch");
+	if (err == NULL && *outsize != result_size)
+		err = got_error_fmt(GOT_ERR_BAD_DELTA,
+		    "delta application result size mismatch: actual: %zd "
+		    "expected: %llu", *outsize, result_size);
 
 	if (memstream != NULL) {
-		if (fclose(memstream) == EOF)
+		if (fclose(memstream) == EOF && err == NULL)
 			err = got_error_from_errno("fclose");
 		if (err == NULL) {
 			size_t n;
blob - 8717d4c083ff9429232d344b13335043e9403933
blob + e7c7b8205182d09c3928dd0df949d85a3222a7b4
--- lib/deltify.c
+++ lib/deltify.c
@@ -504,7 +504,7 @@ stretchblk(FILE *basefile, off_t base_offset0, struct 
 	    SEEK_SET) == -1)
 		return got_error_from_errno("fseeko");
 
-	while (buf_equal && *blocklen < (1 << 24) - 1) {
+	while (buf_equal && *blocklen < GOT_DELTIFY_STRETCHMAX - 1) {
 		base_r = fread(basebuf, 1, sizeof(basebuf), basefile);
 		if (base_r == 0) {
 			if (ferror(basefile))
@@ -522,6 +522,8 @@ stretchblk(FILE *basefile, off_t base_offset0, struct 
 				buf_equal = 0;
 				break;
 			}
+			if (*blocklen >= GOT_DELTIFY_STRETCHMAX)
+				break;
 			(*blocklen)++;
 		}
 	}
@@ -558,6 +560,8 @@ stretchblk_file_mem(uint8_t *basedata, off_t base_offs
 				buf_equal = 0;
 				break;
 			}
+			if (*blocklen >= GOT_DELTIFY_STRETCHMAX)
+				break;
 			(*blocklen)++;
 		}
 	}
@@ -598,6 +602,8 @@ stretchblk_mem_file(FILE *basefile, off_t base_offset0
 				buf_equal = 0;
 				break;
 			}
+			if (*blocklen >= GOT_DELTIFY_STRETCHMAX)
+				break;
 			(*blocklen)++;
 		}
 	}
@@ -629,7 +635,7 @@ stretchblk_mem_mem(uint8_t *basedata, off_t base_offse
 	p = data + fileoffset;
 	q = basedata + base_offset;
 	maxlen = MIN(basefile_size - base_offset, filesize - fileoffset);
-	for (i = 0; i < maxlen && *blocklen < (1 << 24) - 1; i++) {
+	for (i = 0; i < maxlen && *blocklen < GOT_DELTIFY_STRETCHMAX - 1; i++) {
 		if (p[i] != q[i])
 			break;
 		(*blocklen)++;
blob - 973f1682b45bdf0b31cfe37ef1473caa9819418d
blob + 644fb97f03e7d5c3669e32724188d86ee8f5ca27
--- lib/got_lib_deltify.h
+++ lib/got_lib_deltify.h
@@ -45,6 +45,7 @@ enum {
 	GOT_DELTIFY_MINCHUNK	= 32,
 	GOT_DELTIFY_MAXCHUNK	= 8192,
 	GOT_DELTIFY_SPLITMASK	= (1 << 8) - 1,
+	GOT_DELTIFY_STRETCHMAX	= (1 << 24) - 1,
 };
 
 const struct got_error *got_deltify_init(struct got_delta_table **dt, FILE *f,
blob - ff0ad628633ae3a3e6c5bd87cc76f95a70977acb
blob + 1698972f9fb04c46f3e9026125a1493811eb358d
--- regress/gotd/Makefile
+++ regress/gotd/Makefile
@@ -7,7 +7,8 @@ REGRESS_TARGETS=test_repo_read test_repo_read_group \
 	test_repo_write_protected test_repo_write_readonly \
 	test_email_notification test_http_notification \
 	test_git_interop test_email_and_http_notification \
-	test_http_notification_hmac test_connection_limit
+	test_http_notification_hmac test_connection_limit \
+	test_large_loose_objects
 NOOBJ=Yes
 CLEANFILES=gotd.conf gotd-secrets.conf
 
@@ -252,6 +253,10 @@ prepare_test_repo_empty: ensure_root
 	@chown ${GOTD_USER} "${GOTD_TEST_REPO}"
 	@su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./prepare_test_repo.sh 1'
 
+prepare_large_loose_objects: ensure_root prepare_test_repo
+	@su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) \
+		sh ./prepare_large_loose_objects.sh'
+
 test_repo_read: prepare_test_repo start_gotd_ro
 	@-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \
 		'env $(GOTD_TEST_ENV) sh ./repo_read.sh'
@@ -350,4 +355,9 @@ test_connection_limit: prepare_test_repo start_gotd_co
 		'env $(GOTD_TEST_ENV) sh ./connection_limit.sh'
 	@$(GOTD_STOP_CMD) 2>/dev/null
 
+test_large_loose_objects: prepare_large_loose_objects start_gotd_ro
+	@-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \
+		'env $(GOTD_TEST_ENV) sh ./large_loose_objects.sh'
+	@$(GOTD_STOP_CMD) 2>/dev/null
+
 .include <bsd.regress.mk>
blob - /dev/null
blob + 358b001d6584ee02bae559e83d62199d0b6ea030 (mode 644)
--- /dev/null
+++ regress/gotd/large_loose_objects.sh
@@ -0,0 +1,63 @@
+#!/bin/sh
+#
+# Copyright (c) 2025 Stefan Sperling <stsp@openbsd.org>
+#
+# Permission to use, copy, modify, and distribute this software for any
+# purpose with or without fee is hereby granted, provided that the above
+# copyright notice and this permission notice appear in all copies.
+#
+# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+. ../cmdline/common.sh
+. ./common.sh
+
+test_clone_basic() {
+	local testroot=`test_init clone_basic 1`
+
+	got clone -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got clone failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	# Verify that the clone operation worked fine.
+	git_fsck "$testroot" "$testroot/repo-clone"
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	got tree -R -r "$testroot/repo-clone" > $testroot/stdout
+	cat > $testroot/stdout.expected <<EOF
+alpha
+beta
+epsilon/
+epsilon/zeta
+gamma/
+gamma/delta
+large_file1
+large_file2
+large_file3
+EOF
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	test_done "$testroot" "$ret"
+}
+
+test_parseargs "$@"
+run_test test_clone_basic
blob - /dev/null
blob + ead9fd3de77e4ceb9aaec35ff1a5198a9a2e4400 (mode 755)
--- /dev/null
+++ regress/gotd/prepare_large_loose_objects.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+#
+# Copyright (c) 2025 Stefan Sperling <stsp@openbsd.org>
+#
+# Permission to use, copy, modify, and distribute this software for any
+# purpose with or without fee is hereby granted, provided that the above
+# copyright notice and this permission notice appear in all copies.
+#
+# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+test_tree=`mktemp -d "${GOTD_TEST_ROOT}/gotd-test-tree-XXXXXXXXXX"`
+trap "rm -r $test_tree" HUP INT QUIT PIPE TERM
+
+got checkout -q "${GOTD_TEST_REPO}" "$test_tree" > /dev/null
+
+dd if=/dev/random of="$test_tree/large_file1" count=32768 status=none
+(cd "$test_tree" && got add "$test_tree/large_file1" > /dev/null)
+for i in 2 3; do
+       cp "$test_tree/large_file1" "$test_tree/large_file$i"
+       dd if=/dev/random of="$test_tree/large_file$i" seek=32768 count=64 \
+              status=none
+       (cd "$test_tree" && got add "$test_tree/large_file$i" > /dev/null)
+done
+
+(cd "$test_tree" && got commit -m "add large objects" "$test_tree" > /dev/null)
+
+rm -r "$test_tree"