[PATCH] cache-tree: replace a sscanf() by two strtol() calls

Johannes Schindelin· May 2, 2006, 01:31 UTC · lore

On one of my systems, sscanf() first calls strlen() on the buffer. But this buffer is not terminated by NUL. So git crashed.

strtol() does not share that problem, as it stops reading after the first non-digit.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

	Maybe, a better solution would be to store the integers in 
	binary form. But I am not familiar with that part of git, and
	further, it would break setups which already have an index
	with cache-tree information.

 cache-tree.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

4325bb506d03a0e30a5d4dd197601a53f0375df9

Show changes to cache-tree.c +10 −1

diff --git a/cache-tree.c b/cache-tree.c
index 28b78f8..bd7c1aa 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -439,6 +439,7 @@ void *cache_tree_write(struct cache_tree
 static struct cache_tree *read_one(const char **buffer, unsigned long *size_p)
 {
 	const char *buf = *buffer;
+	char *endptr;
 	unsigned long size = *size_p;
 	struct cache_tree *it;
 	int i, subtree_nr;
@@ -453,8 +454,16 @@ static struct cache_tree *read_one(const
 		goto free_return;
 	buf++; size--;
 	it = cache_tree();
-	if (sscanf(buf, "%d %d\n", &it->entry_count, &subtree_nr) != 2)
+	it->entry_count = strtol(buf, &endptr, 10);
+	if (buf == endptr)
 		goto free_return;
+	size -= (endptr - buf);
+	buf = endptr + 1;
+	subtree_nr = strtol(buf, &endptr, 10);
+	if (buf == endptr)
+		goto free_return;
+	size -= (endptr - buf);
+	buf = endptr + 1;
 	while (size && *buf && *buf != '\n') {
 		size--;
 		buf++;

-- 
1.3.1.g5d53-dirty

Junio C Hamano· May 2, 2006, 23:26 UTC · re: Johannes Schindelin · lore

Re: [PATCH] cache-tree: replace a sscanf() by two strtol() calls

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On one of my systems, sscanf() first calls strlen() on the buffer. But
> this buffer is not terminated by NUL. So git crashed.

Interesting.

"git grep sscanf next -- '*.c'" reports handful sscanf() there. Most of them scan argv[], which are NUL terminated, so they should be OK. The rest in convert-objects.c, tree-walk.c, and tree.c all scan the mode bits in tree objects, which will later have the pathname component terminated with NUL, so that would also be OK.

But I think your patch is wrong, and makes it ignore cache-tree structure; I suspect you have two off-by-one errors and are making buf and size out of sync.

> 	Maybe, a better solution would be to store the integers in 
> 	binary form. But I am not familiar with that part of git, and
> 	further, it would break setups which already have an index
> 	with cache-tree information.

In theory, it is stashed in the extension section of index so we could define a new extension type, say "TRE2" and store the information in the new format. But this is purely a cache used as optimiation, so we could just say, "make sure to save local modifications before doing an update, then run 'rm .git/index && git-read-tree HEAD' please".

I've applied a fixed up one, but I am actually thinking about
ripping out the whole cache-tree thing and redoing it all in the
index.
 
Currently the index stores set of blobs after flattening the
hierarchical tree structure, losing the original "tree"
information.  We could instead store something that looks like
"ls-tree -t -r" output (plus the toplevel tree information which
"ls-tree -t -r" does not give you).  Just like an update-index
on the path t/t0000-basic.sh invalidates the cache-tree entry
for "" and "t/", we could either invalidate or recompute (I am
inclined to do the former to make things lazy) these "tree"
entries in the index.  This would be more direct way to store
what I am storing in the cache-tree.

Keeping the object names of unchanged subtrees available will allow us to walk the index and a tree (or two or more trees) in parallel in various applications. "diff-index --cached" and "read-tree -m" extract one entry from tree and index for each blob, but when we have an up-to-date information for a subtree in the index, and when that subtree matches the corresponding subtree in the tree object diff-index or read-tree is reading, the application can short-cut without reading anything in the subtree.

patchcache-tree: replace a sscanf() by two strtol() calls