[PATCH 0/2] tree-walk: use size_t for tree sizes

[PATCH 0/2] tree-walk: use size_t for tree sizes

[Elia Pinto]

struct tree_desc keeps the size of the tree buffer in an "unsigned
int", so a tree object of 4GiB or more is truncated on all platforms,
and struct tree adds another truncation on Windows, where "long" is
32 bits. The walk then sees only a prefix of the tree: it never reads
past the buffer, but when the cut falls between two entries the rest
of the tree silently disappears, both for commands like "git ls-tree"
and for the checks in fsck_tree().

Trees that large are not something one creates by accident, but they
can be crafted, and fsck is what is supposed to look at every entry
of them.

 1/2 moves struct tree_desc and the functions that set it up to
     size_t; this is the part that matters on all platforms.
 2/2 does the same for struct tree and parse_tree_buffer().

There is no test, as it would need a tree larger than 4GiB in memory;
1/2 describes how I checked the effect with a simulated 16-bit size.

This comes from the same search for truncated sizes as:

  https://lore.kernel.org/git/cover.1793352943.git.gitter.spiros@gmail.com/
  https://lore.kernel.org/git/cover.1793355390.git.gitter.spiros@gmail.com/

It is independent of both and merges cleanly with them. The series is
based on master (b9e5cff180).

Elia Pinto (2):
  tree-walk: use size_t for the size of tree buffers
  tree: use size_t for the size of parsed trees

 tree-walk.c | 14 +++++++-------
 tree-walk.h |  6 +++---
 tree.c      |  2 +-
 tree.h      |  4 ++--
 4 files changed, 13 insertions(+), 13 deletions(-)


base-commit: b9e5cff1803ab24582201ef059516eb16014b9d9
-- 
2.53.0

[PATCH 1/2] tree-walk: use size_t for the size of tree buffers

[Elia Pinto]

struct tree_desc keeps the number of bytes left in the tree buffer in
an "unsigned int", and init_tree_desc(), init_tree_desc_gently() and
decode_tree_entry() take the size as an "unsigned long". The size
comes from odb_read_object() as a size_t, so for a tree object of
4GiB or more it is truncated. As the field is an "unsigned int", this
happens on all platforms, not only those where "long" is 32 bits.

The walk then sees only the first "size mod 2^32" bytes of the
buffer. decode_tree_entry() still checks that the NUL ending the last
entry is within that size, so we never read past the buffer. But when
the cut falls between two entries, the rest of the tree silently
disappears: "git ls-tree" lists fewer entries and exits with 0, and
fsck_tree() only checks the entries it sees, so its checks for ".git",
".gitmodules" and the like never look at the others. When the cut
falls inside an entry, we die with "too-short tree" instead.

Use size_t for the size in struct tree_desc, in the functions that set
it up and advance it, and in the struct dir_state used by
get_tree_entry_follow_symlinks().

There is no test, as it would need a tree larger than 4GiB in memory.
Truncating the size to 16 bits instead, in init_tree_desc_internal()
and update_tree_entry_internal(), makes "git ls-tree" list 10 of the
2058 entries of a 65856-byte tree created with "git mktree", without
any error.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 tree-walk.c | 14 +++++++-------
 tree-walk.h |  6 +++---
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tree-walk.c b/tree-walk.c
index a67f06b9eb..aaac1fd48d 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -14,7 +14,7 @@
 #include "environment.h"
 #include "read-cache-ll.h"
 
-static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size, struct strbuf *err)
+static int decode_tree_entry(struct tree_desc *desc, const char *buf, size_t size, struct strbuf *err)
 {
 	const char *path;
 	unsigned int len;
@@ -49,7 +49,7 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l
 
 static int init_tree_desc_internal(struct tree_desc *desc,
 				   const struct object_id *oid,
-				   const void *buffer, unsigned long size,
+				   const void *buffer, size_t size,
 				   struct strbuf *err,
 				   enum tree_desc_flags flags)
 {
@@ -63,7 +63,7 @@ static int init_tree_desc_internal(struct tree_desc *desc,
 }
 
 void init_tree_desc(struct tree_desc *desc, const struct object_id *tree_oid,
-		    const void *buffer, unsigned long size)
+		    const void *buffer, size_t size)
 {
 	struct strbuf err = STRBUF_INIT;
 	if (init_tree_desc_internal(desc, tree_oid, buffer, size, &err, 0))
@@ -72,7 +72,7 @@ void init_tree_desc(struct tree_desc *desc, const struct object_id *tree_oid,
 }
 
 int init_tree_desc_gently(struct tree_desc *desc, const struct object_id *oid,
-			  const void *buffer, unsigned long size,
+			  const void *buffer, size_t size,
 			  enum tree_desc_flags flags)
 {
 	struct strbuf err = STRBUF_INIT;
@@ -113,8 +113,8 @@ static int update_tree_entry_internal(struct tree_desc *desc, struct strbuf *err
 {
 	const void *buf = desc->buffer;
 	const unsigned char *end = (const unsigned char *)desc->entry.path + desc->entry.pathlen + 1 + desc->algo->rawsz;
-	unsigned long size = desc->size;
-	unsigned long len = end - (const unsigned char *)buf;
+	size_t size = desc->size;
+	size_t len = end - (const unsigned char *)buf;
 
 	if (size < len)
 		die(_("too-short tree file"));
@@ -561,7 +561,7 @@ int traverse_trees(struct index_state *istate,
 
 struct dir_state {
 	void *tree;
-	unsigned long size;
+	size_t size;
 	struct object_id oid;
 };
 
diff --git a/tree-walk.h b/tree-walk.h
index 9646c47ac5..5ccd4a9361 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -35,7 +35,7 @@ struct tree_desc {
 	struct name_entry entry;
 
 	/* counts the number of bytes left in the `buffer`. */
-	unsigned int size;
+	size_t size;
 
 	/* option flags passed via init_tree_desc_gently() */
 	enum tree_desc_flags {
@@ -85,10 +85,10 @@ int update_tree_entry_gently(struct tree_desc *);
  * members of `struct tree`.
  */
 void init_tree_desc(struct tree_desc *desc, const struct object_id *tree_oid,
-		    const void *buf, unsigned long size);
+		    const void *buf, size_t size);
 
 int init_tree_desc_gently(struct tree_desc *desc, const struct object_id *oid,
-			  const void *buf, unsigned long size,
+			  const void *buf, size_t size,
 			  enum tree_desc_flags flags);
 
 /*
-- 
2.53.0

[PATCH 2/2] tree: use size_t for the size of parsed trees

[Elia Pinto]

struct tree stores the size of its buffer in an "unsigned long", which
is only 32 bits wide on Windows, and parse_tree_buffer() takes it as
such, although repo_parse_tree_gently() gets it from odb_read_object()
as a size_t. Callers then pass tree->size to init_tree_desc(), so with
the previous commit a tree of 4GiB or more is still truncated on
Windows before it gets there.

Use size_t for both. parse_object_buffer() still hands an "unsigned
long" size to parse_tree_buffer(); it is shared with the other object
types and is left for a later change.

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

diff --git a/tree.c b/tree.c
index 53f7395e9f..d37b9bc7b1 100644
--- a/tree.c
+++ b/tree.c
@@ -172,7 +172,7 @@ struct tree *lookup_tree(struct repository *r, const struct object_id *oid)
 	return object_as_type(obj, OBJ_TREE, 0);
 }
 
-int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size)
+int parse_tree_buffer(struct tree *item, void *buffer, size_t size)
 {
 	if (item->object.parsed)
 		return 0;
diff --git a/tree.h b/tree.h
index 677382eed8..50f0b15af4 100644
--- a/tree.h
+++ b/tree.h
@@ -10,14 +10,14 @@ struct strbuf;
 struct tree {
 	struct object object;
 	void *buffer;
-	unsigned long size;
+	size_t size;
 };
 
 extern const char *tree_type;
 
 struct tree *lookup_tree(struct repository *r, const struct object_id *oid);
 
-int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size);
+int parse_tree_buffer(struct tree *item, void *buffer, size_t size);
 
 #define parse_tree_gently(t, q) repo_parse_tree_gently(the_repository, t, q)
 int repo_parse_tree_gently(struct repository *r, struct tree *item,
-- 
2.53.0