Add a new function "free_object_buffer", which marks the object as
un-parsed and frees the buffer. Only trees and commits have buffers;
other types are not affected. If the tree or commit buffer is already
NULL, the "parsed" flag is still cleared so callers can control the free
themselves (index-pack.c uses this).
Several areas of code would free buffers for object structs that
contained them ("struct tree" and "struct commit"), but without clearing
the "parsed" flag. parse_object would clear the flag for "struct tree",
but commits would remain in an invalid state (marked as parsed but with
a NULL buffer). Because the objects are uniqued (ccdc6037fee), the
invalid objects stay around and can lead to bad behavior.
In particular, running pickaxe on all refs in a repository with a cached
textconv could segfault. If the textconv cache ref was evaluated first
by cmd_log_walk, a subsequent notes_cache_match_validity call would
dereference NULL.
Signed-off-by: Jonathon Mah <me@xxxxxxxxxxxxxxx>
---
I found an old email where Jeff noted that this would be bad (yet the buffer manipulation remained).
<http://permalink.gmane.org/gmane.comp.version-control.git/188000>
builtin/fsck.c | 17 ++---------------
builtin/index-pack.c | 3 +++
builtin/log.c | 9 +++------
builtin/reflog.c | 3 +--
builtin/rev-list.c | 3 +--
http-push.c | 3 +--
list-objects.c | 3 +--
object.c | 25 +++++++++++++++++++++++--
object.h | 3 +++
reachable.c | 3 +--
revision.c | 3 +--
t/t4042-diff-textconv-caching.sh | 11 +++++++++++
upload-pack.c | 3 +--
walker.c | 5 +----
14 files changed, 53 insertions(+), 41 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index bb9a2cd..82b3612 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -133,10 +133,7 @@ static int traverse_one_object(struct object *obj)
return 1; /* error already displayed */
}
result = fsck_walk(obj, mark_object, obj);
- if (tree) {
- free(tree->buffer);
- tree->buffer = NULL;
- }
+ free_object_buffer(obj);
return result;
}
@@ -303,26 +300,16 @@ static int fsck_obj(struct object *obj)
if (fsck_object(obj, check_strict, fsck_error_func))
return -1;
- if (obj->type == OBJ_TREE) {
- struct tree *item = (struct tree *) obj;
-
- free(item->buffer);
- item->buffer = NULL;
- }
+ free_object_buffer(obj);
if (obj->type == OBJ_COMMIT) {
struct commit *commit = (struct commit *) obj;
-
- free(commit->buffer);
- commit->buffer = NULL;
-
if (!commit->parents && show_root)
printf("root %s\n", sha1_to_hex(commit->object.sha1));
}
if (obj->type == OBJ_TAG) {
struct tag *tag = (struct tag *) obj;
-
if (show_tags && tag->tagged) {
printf("tagged %s %s", typename(tag->tagged->type), sha1_to_hex(tag->tagged->sha1));
printf(" (%s) in %s\n", tag->tag, sha1_to_hex(tag->object.sha1));
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 43d364b..0eb39ae 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -750,13 +750,16 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
if (fsck_walk(obj, mark_link, NULL))
die(_("Not all child objects of %s are reachable"), sha1_to_hex(obj->sha1));
+ /* set buffer to NULL so it isn't freed */
if (obj->type == OBJ_TREE) {
struct tree *item = (struct tree *) obj;
item->buffer = NULL;
+ free_object_buffer(obj);
}
if (obj->type == OBJ_COMMIT) {
struct commit *commit = (struct commit *) obj;
commit->buffer = NULL;
+ free_object_buffer(obj);
}
obj->flags |= FLAG_CHECKED;
}
diff --git a/builtin/log.c b/builtin/log.c
index e7b7db1..433b874 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -305,11 +305,9 @@ static int cmd_log_walk(struct rev_info *rev)
* but we didn't actually show the commit.
*/
rev->max_count++;
- if (!rev->reflog_info) {
+ if (!rev->reflog_info)
/* we allow cycles in reflog ancestry */
- free(commit->buffer);
- commit->buffer = NULL;
- }
+ free_object_buffer(&commit->object);
free_commit_list(commit->parents);
commit->parents = NULL;
if (saved_nrl < rev->diffopt.needed_rename_limit)
@@ -1399,8 +1397,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
reopen_stdout(numbered_files ? NULL : commit, NULL, &rev, quiet))
die(_("Failed to create output files"));
shown = log_tree_commit(&rev, commit);
- free(commit->buffer);
- commit->buffer = NULL;
+ free_object_buffer(&commit->object);
/* We put one extra blank line between formatted
* patches and this flag is used by log-tree code
diff --git a/builtin/reflog.c b/builtin/reflog.c
index b3c9e27..c9a660f 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -94,8 +94,7 @@ static int tree_is_complete(const unsigned char *sha1)
complete = 0;
}
}
- free(tree->buffer);
- tree->buffer = NULL;
+ free_object_buffer(tree->buffer);
if (complete)
tree->object.flags |= SEEN;
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 67701be..6855892 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -170,8 +170,7 @@ static void finish_commit(struct commit *commit, void *data)
free_commit_list(commit->parents);
commit->parents = NULL;
}
- free(commit->buffer);
- commit->buffer = NULL;
+ free_object_buffer(&commit->object);
}
static void finish_object(struct object *obj,
diff --git a/http-push.c b/http-push.c
index 8701c12..042a410 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1347,8 +1347,7 @@ static struct object_list **process_tree(struct tree *tree,
break;
}
- free(tree->buffer);
- tree->buffer = NULL;
+ free_object_buffer(obj);
return p;
}
diff --git a/list-objects.c b/list-objects.c
index 3dd4a96..fb4b531 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -123,8 +123,7 @@ static void process_tree(struct rev_info *revs,
cb_data);
}
strbuf_setlen(base, baselen);
- free(tree->buffer);
- tree->buffer = NULL;
+ free_object_buffer(obj);
}
static void mark_edge_parents_uninteresting(struct commit *commit,
diff --git a/object.c b/object.c
index 4af3451..8e161f8 100644
--- a/object.c
+++ b/object.c
@@ -149,8 +149,6 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
struct tree *tree = lookup_tree(sha1);
if (tree) {
obj = &tree->object;
- if (!tree->buffer)
- tree->object.parsed = 0;
if (!tree->object.parsed) {
if (parse_tree_buffer(tree, buffer, size))
return NULL;
@@ -225,6 +223,29 @@ struct object *parse_object(const unsigned char *sha1)
return NULL;
}
+void free_object_buffer(struct object *item)
+{
+ if (!item)
+ return;
+
+ if (item->type == OBJ_TREE) {
+ struct tree *tree = (struct tree *)item;
+ tree->object.parsed = 0;
+ tree->size = 0;
+ if (tree->buffer) {
+ free(tree->buffer);
+ tree->buffer = NULL;
+ }
+ } else if (item->type == OBJ_COMMIT) {
+ struct commit *commit = (struct commit *)item;
+ commit->object.parsed = 0;
+ if (commit->buffer) {
+ free(commit->buffer);
+ commit->buffer = NULL;
+ }
+ }
+}
+
struct object_list *object_list_insert(struct object *item,
struct object_list **list_p)
{
diff --git a/object.h b/object.h
index 6a97b6b..cbc730c 100644
--- a/object.h
+++ b/object.h
@@ -63,6 +63,9 @@ struct object *parse_object(const unsigned char *sha1);
*/
struct object *parse_object_buffer(const unsigned char *sha1, enum object_type type, unsigned long size, void *buffer, int *eaten_p);
+/** If the object's type has a buffer, it's freed and marked as un-parsed. */
+void free_object_buffer(struct object *item);
+
/** Returns the object, with potentially excess memory allocated. **/
struct object *lookup_unknown_object(const unsigned char *sha1);
diff --git a/reachable.c b/reachable.c
index bf79706..c29d3e0 100644
--- a/reachable.c
+++ b/reachable.c
@@ -80,8 +80,7 @@ static void process_tree(struct tree *tree,
else
process_blob(lookup_blob(entry.sha1), p, &me, entry.path, cp);
}
- free(tree->buffer);
- tree->buffer = NULL;
+ free_object_buffer(obj);
}
static void process_tag(struct tag *tag, struct object_array *p,
diff --git a/revision.c b/revision.c
index 95d21e6..43c5eec 100644
--- a/revision.c
+++ b/revision.c
@@ -133,8 +133,7 @@ void mark_tree_uninteresting(struct tree *tree)
* We don't care about the tree any more
* after it has been marked uninteresting.
*/
- free(tree->buffer);
- tree->buffer = NULL;
+ free_object_buffer(obj);
}
void mark_parents_uninteresting(struct commit *commit)
diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index 91f8198..d7e26ca 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -106,4 +106,15 @@ test_expect_success 'switching diff driver produces correct results' '
test_cmp expect actual
'
+cat >expect <<EOF
+./helper other (refs/notes/textconv/magic)
+one
+EOF
+# add empty commit on master to make bug more reproducible
+test_expect_success 'pickaxe with cached textconv' '
+ git commit --allow-empty -m another &&
+ git log -S other --pretty=tformat:%s%d --all >actual &&
+ test_cmp expect actual
+'
+
test_done
diff --git a/upload-pack.c b/upload-pack.c
index 6142421..1feacbc 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -81,8 +81,7 @@ static void show_commit(struct commit *commit, void *data)
die("broken output pipe");
fputc('\n', pack_pipe);
fflush(pack_pipe);
- free(commit->buffer);
- commit->buffer = NULL;
+ free_object_buffer(&commit->object);
}
static void show_object(struct object *obj,
diff --git a/walker.c b/walker.c
index be389dc..bae4876 100644
--- a/walker.c
+++ b/walker.c
@@ -56,10 +56,7 @@ static int process_tree(struct walker *walker, struct tree *tree)
if (!obj || process(walker, obj))
return -1;
}
- free(tree->buffer);
- tree->buffer = NULL;
- tree->size = 0;
- tree->object.parsed = 0;
+ free_object_buffer(&tree->object);
return 0;
}
--
1.8.1.1
Jonathon Mah
me@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html