[PATCH 0/4] use size_t when receiving and verifying packs

[PATCH 0/4] use size_t when receiving and verifying packs

[Elia Pinto]

This series continues the search for truncated sizes, this time focusing
on the side that receives and verifies packs: unpack-objects,
index-pack, and pack verification in git fsck. These components keep
object sizes as "unsigned long", while the underlying pack code already
uses size_t.

Each patch addresses a specific part of the pack pipeline:
 - 1/4 unpack-objects: on Windows, it dies when receiving an object of
   4GiB or more;
 - 2/4 index-pack write_compressed(): on all platforms ("unsigned int"),
   --fix-thin with a local base of 4GiB or more dies with "serious
   inflate inconsistency", verified by simulating at 16 bits;
 - 3/4 index-pack: on Windows, it triggers a false "SHA1 COLLISION
   FOUND" when the object already exists locally;
 - 4/4 pack-check: on Windows, "git fsck" checks a prefix of objects in
   packs.

These are all failures (die) or checks on a prefix; no corrupt data ends
up being written.

No tests are added, for the same reason as the object parsing series
(requiring 4GiB objects and 32-bit long platforms, or objects of 4GiB on
all platforms for 2/4).

This series closes the pack-verification boundary left open for
fsck_object() by the object parsing and checking series:
  https://lore.kernel.org/git/cover.1793444420.git.gitter.spiros@gmail.com/
It does not depend on other in-flight series and merges cleanly with
them. For the full effect on non-streamed objects in 1/4, the downstream
fsck_object() / parse_object_buffer() (from the series above) and
odb_write_object() (from v3 of "use size_t for a few more object and
buffer sizes") are needed as well:
  https://lore.kernel.org/git/cover.1793439137.git.gitter.spiros@gmail.com/
Pack-objects (delta search below core.bigFileThreshold) is left for a
later series.

Based on master 50fe5cc63f.

Elia Pinto (4):
  unpack-objects: use size_t for object sizes
  index-pack: use size_t in write_compressed()
  index-pack: use size_t for object sizes
  pack-check: use size_t for the size passed to verify callbacks

 builtin/fsck.c           |  2 +-
 builtin/index-pack.c     | 14 +++++++-------
 builtin/unpack-objects.c | 36 +++++++++++++++++-------------------
 odb.h                    |  2 +-
 pack.h                   |  2 +-
 5 files changed, 27 insertions(+), 29 deletions(-)


base-commit: 50fe5cc63f9cd6e89b87a2c9ccf96a444f4cfdb7
-- 
2.53.0

[PATCH 1/4] unpack-objects: use size_t for object sizes

[Elia Pinto]

builtin/unpack-objects.c uses "unsigned long" to track object sizes
throughout the file, while sizes read from packs are handled as size_t.
On platforms where "long" is 32 bits, this truncates the size for
objects of 4GiB or more.

When handling large objects on those platforms, unpack-objects fails
rather than corrupting data. For instance, streaming large blobs
(exceeding core.bigFileThreshold) passes a truncated size to
stream_blob(), causing the stream write to fail with an error.

Use size_t across the board in builtin/unpack-objects.c and remove the
now-unnecessary size cast in unpack_delta_entry(). Downstream consumers
like odb_write_object() and fsck_object() are updated separately.

Triggering this requires processing 4GiB or more of input on a platform
where "long" is 32 bits, so no test is added.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 builtin/unpack-objects.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 351948724a..6c830a606f 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -40,7 +40,7 @@ static struct progress *progress;
  */
 struct obj_buffer {
 	char *buffer;
-	unsigned long size;
+	size_t size;
 };
 
 static struct decoration obj_decorate;
@@ -50,7 +50,7 @@ static struct obj_buffer *lookup_object_buffer(struct object *base)
 	return lookup_decoration(&obj_decorate, base);
 }
 
-static void add_object_buffer(struct object *object, char *buffer, unsigned long size)
+static void add_object_buffer(struct object *object, char *buffer, size_t size)
 {
 	struct obj_buffer *obj;
 	CALLOC_ARRAY(obj, 1);
@@ -114,10 +114,10 @@ static void use(int bytes)
  * allocated buffer which is reused to hold temporary zstream output
  * and return NULL instead of returning garbage data.
  */
-static void *get_data(unsigned long size)
+static void *get_data(size_t size)
 {
 	git_zstream stream;
-	unsigned long bufsize = dry_run && size > 8192 ? 8192 : size;
+	size_t bufsize = dry_run && size > 8192 ? 8192 : size;
 	void *buf = xmallocz(bufsize);
 
 	memset(&stream, 0, sizeof(stream));
@@ -161,7 +161,7 @@ struct delta_info {
 	struct object_id base_oid;
 	unsigned nr;
 	off_t base_offset;
-	unsigned long size;
+	size_t size;
 	void *delta;
 	struct delta_info *next;
 };
@@ -170,7 +170,7 @@ static struct delta_info *delta_list;
 
 static void add_delta_to_list(unsigned nr, const struct object_id *base_oid,
 			      off_t base_offset,
-			      void *delta, unsigned long size)
+			      void *delta, size_t size)
 {
 	struct delta_info *info = xmalloc(sizeof(*info));
 
@@ -261,7 +261,7 @@ static void write_rest(void)
 }
 
 static void added_object(unsigned nr, enum object_type type,
-			 void *data, unsigned long size);
+			 void *data, size_t size);
 
 /*
  * Write out nr-th object from the list, now we know the contents
@@ -269,7 +269,7 @@ static void added_object(unsigned nr, enum object_type type,
  * to be checked at the end.
  */
 static void write_object(unsigned nr, enum object_type type,
-			 void *buf, unsigned long size)
+			 void *buf, size_t size)
 {
 	if (!strict) {
 		if (odb_write_object(the_repository->objects, buf, size, type,
@@ -310,8 +310,8 @@ static void write_object(unsigned nr, enum object_type type,
 }
 
 static void resolve_delta(unsigned nr, enum object_type type,
-			  void *base, unsigned long base_size,
-			  void *delta, unsigned long delta_size)
+			  void *base, size_t base_size,
+			  void *delta, size_t delta_size)
 {
 	void *result;
 	size_t result_size;
@@ -330,7 +330,7 @@ static void resolve_delta(unsigned nr, enum object_type type,
  * resolve all the deltified objects that are based on it.
  */
 static void added_object(unsigned nr, enum object_type type,
-			 void *data, unsigned long size)
+			 void *data, size_t size)
 {
 	struct delta_info **p = &delta_list;
 	struct delta_info *info;
@@ -349,7 +349,7 @@ static void added_object(unsigned nr, enum object_type type,
 	}
 }
 
-static void unpack_non_delta_entry(enum object_type type, unsigned long size,
+static void unpack_non_delta_entry(enum object_type type, size_t size,
 				   unsigned nr)
 {
 	void *buf = get_data(size);
@@ -386,7 +386,7 @@ static ssize_t zlib_stream_read(struct odb_stream *in_stream,
 	return buf_len - zstream->avail_out;
 }
 
-static void stream_blob(unsigned long size, unsigned nr)
+static void stream_blob(size_t size, unsigned nr)
 {
 	git_zstream zstream = { 0 };
 	struct zlib_stream in_stream = {
@@ -420,7 +420,7 @@ static void stream_blob(unsigned long size, unsigned nr)
 }
 
 static int resolve_against_held(unsigned nr, const struct object_id *base,
-				void *delta_data, unsigned long delta_size)
+				void *delta_data, size_t delta_size)
 {
 	struct object *obj;
 	struct obj_buffer *obj_buffer;
@@ -435,12 +435,11 @@ static int resolve_against_held(unsigned nr, const struct object_id *base,
 	return 1;
 }
 
-static void unpack_delta_entry(enum object_type type, unsigned long delta_size,
+static void unpack_delta_entry(enum object_type type, size_t delta_size,
 			       unsigned nr)
 {
 	void *delta_data, *base;
-	unsigned long base_size;
-	size_t base_size_st = 0;
+	size_t base_size = 0;
 	struct object_id base_oid;
 
 	if (type == OBJ_REF_DELTA) {
@@ -517,8 +516,7 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,
 		return;
 
 	base = odb_read_object(the_repository->objects, &base_oid,
-			       &type, &base_size_st);
-	base_size = cast_size_t_to_ulong(base_size_st);
+			       &type, &base_size);
 	if (!base) {
 		error("failed to read delta-pack base object %s",
 		      oid_to_hex(&base_oid));
-- 
2.53.0

[PATCH 2/4] index-pack: use size_t in write_compressed()

[Elia Pinto]

write_compressed() in builtin/index-pack.c takes the object size as an
"unsigned int" and returns an "int", which are 32 bits wide on all
platforms. It is used by append_obj_to_pack() when resolving thin packs
with --fix-thin.

With a local base of 4GiB or more, write_compressed() compresses only
the first "size mod 2^32" bytes, while the pack header written by
append_obj_to_pack() records the full size. Later, when resolving deltas
against that base, unpack_data() encounters an early end of stream and
dies with "serious inflate inconsistency". Shrinking the parameter to 16
bits and using a 100KiB base makes "git index-pack --fix-thin" die the
same way.

Use size_t for both the size parameter and the return value.

Triggering this requires processing a local base of 4GiB or more, so
no test is added.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 builtin/index-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 0264c18c5f..f65228468b 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1411,7 +1411,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha
 		    nr_ofs_deltas + nr_ref_deltas - nr_resolved_deltas);
 }
 
-static int write_compressed(struct hashfile *f, void *in, unsigned int size)
+static size_t write_compressed(struct hashfile *f, void *in, size_t size)
 {
 	git_zstream stream;
 	int status;
-- 
2.53.0

[PATCH 3/4] index-pack: use size_t for object sizes

[Elia Pinto]

Functions like sha1_object() and append_obj_to_pack() in
builtin/index-pack.c take object sizes as an "unsigned long", even
though struct object_entry already tracks them as size_t. On platforms
where "long" is 32 bits, this truncates the size for objects of 4GiB or
more.

When receiving, on those platforms, a large object that already exists
locally, sha1_object() compares the real size (size_t) against the
truncated size, resulting in a false "SHA1 COLLISION FOUND" error and
aborting index-pack.

Use size_t for the size parameter across these functions.

Triggering this requires processing 4GiB or more of input on a platform
where "long" is 32 bits, so no test is added.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 builtin/index-pack.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index f65228468b..d4c073654d 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -588,7 +588,7 @@ static void *unpack_raw_entry(struct object_entry *obj,
 }
 
 static void *unpack_data(struct object_entry *obj,
-			 int (*consume)(const unsigned char *, unsigned long, void *),
+			 int (*consume)(const unsigned char *, size_t, void *),
 			 void *cb_data)
 {
 	off_t from = obj[0].idx.offset + obj[0].hdr_size;
@@ -765,10 +765,10 @@ struct compare_data {
 	struct object_entry *entry;
 	struct odb_stream *st;
 	unsigned char *buf;
-	unsigned long buf_size;
+	size_t buf_size;
 };
 
-static int compare_objects(const unsigned char *buf, unsigned long size,
+static int compare_objects(const unsigned char *buf, size_t size,
 			   void *cb_data)
 {
 	struct compare_data *data = cb_data;
@@ -881,7 +881,7 @@ static void do_record_outgoing_links(struct object *obj)
 }
 
 static void sha1_object(const void *data, struct object_entry *obj_entry,
-			unsigned long size, enum object_type type,
+			size_t size, enum object_type type,
 			const struct object_id *oid)
 {
 	void *new_data = NULL;
@@ -1438,11 +1438,11 @@ static size_t write_compressed(struct hashfile *f, void *in, size_t size)
 
 static struct object_entry *append_obj_to_pack(struct hashfile *f,
 			       const unsigned char *sha1, void *buf,
-			       unsigned long size, enum object_type type)
+			       size_t size, enum object_type type)
 {
 	struct object_entry *obj = &objects[nr_objects++];
 	unsigned char header[10];
-	unsigned long s = size;
+	size_t s = size;
 	int n = 0;
 	unsigned char c = (type << 4) | (s & 15);
 	s >>= 4;
-- 
2.53.0

[PATCH 4/4] pack-check: use size_t for the size passed to verify callbacks

[Elia Pinto]

verify_pack() in pack-check.c passes each object to a verify_fn callback
(defined in pack.h) taking the object size as an "unsigned long", even
though pack-check already handles it as a size_t. This signature is also
replicated in struct odb_fsck_options (odb.h) and implemented by
fsck_obj_buffer() in builtin/fsck.c.

On platforms where "long" is 32 bits, running "git fsck" on packs with
objects of 4GiB or more passes a truncated size to fsck_obj_buffer(),
causing parse_object_buffer() and fsck_object() to inspect only a
prefix.

Use size_t for the size parameter in verify_fn,
struct odb_fsck_options.object_cb, and fsck_obj_buffer().

Triggering this requires processing objects of 4GiB or more on a
platform where "long" is 32 bits, so no test is added.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 builtin/fsck.c | 2 +-
 odb.h          | 2 +-
 pack.h         | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index da90cf84bf..9f62aa1f4e 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -390,7 +390,7 @@ static void check_connectivity(struct repository *repo)
 }
 
 static int fsck_obj_buffer(const struct object_id *oid, enum object_type type,
-			   unsigned long size, void *buffer, int *eaten, void *cb_data)
+			   size_t size, void *buffer, int *eaten, void *cb_data)
 {
 	struct repository *repo = cb_data;
 	struct object *obj;
diff --git a/odb.h b/odb.h
index dd85b880a7..9cb36ed5ad 100644
--- a/odb.h
+++ b/odb.h
@@ -197,7 +197,7 @@ struct odb_fsck_options {
 	enum odb_fsck_flags flags;
 
 	int (*object_cb)(const struct object_id *oid, enum object_type type,
-			 unsigned long size, void *buffer, int *eaten, void *cb_data);
+			 size_t size, void *buffer, int *eaten, void *cb_data);
 	void *object_payload;
 };
 
diff --git a/pack.h b/pack.h
index ada506b5c5..d9649c6ee0 100644
--- a/pack.h
+++ b/pack.h
@@ -88,7 +88,7 @@ struct progress;
 /* Note, the data argument could be NULL if object type is blob */
 typedef int (*verify_fn)(const struct object_id *oid,
 			 enum object_type type,
-			 unsigned long size,
+			 size_t size,
 			 void *buffer, int *eaten,
 			 void *fn_data);
 
-- 
2.53.0