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

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

[Johannes Schindelin]

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
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

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

[Junio C Hamano]

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.