clear parsed flag when we free tree buffers

Many code paths will free a tree object's buffer and set it
to NULL after finishing with it in order to keep memory
usage down during a traversal. However, out of 8 sites that
do this, only one actually unsets the "parsed" flag back.
Those sites that don't are setting a trap for later users of
the tree object; even after calling parse_tree, the buffer
will remain NULL, causing potential segfaults.

It is not known whether this is triggerable in the current
code. Most commands do not do an in-memory traversal
followed by actually using the objects again. However, it
does not hurt to be safe for future callers.

In most cases, we can abstract this out to a
"free_tree_buffer" helper. However, there are two
exceptions:

1. The fsck code relies on the parsed flag to know that we
were able to parse the object at one point. We can
switch this to using a flag in the "flags" field.

2. The index-pack code sets the buffer to NULL but does
not free it (it is freed by a caller). We should still
unset the parsed flag here, but we cannot use our
helper, as we do not want to free the buffer.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/fsck.c b/builtin/fsck.c
index bb9a2cd44722dc27d54aa5451278a10f512becb8..579fdcceb9a7c1a78767144f66f5bd21a65cdcb1 100644 (file)
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -16,6 +16,7 @@
 
 #define REACHABLE 0x0001
 #define SEEN      0x0002
 
 #define REACHABLE 0x0001
 #define SEEN      0x0002

+#define HAS_OBJ   0x0004

 
 static int show_root;
 static int show_tags;
 
 static int show_root;
 static int show_tags;


@@ -101,7 +102,7 @@ static int mark_object(struct object *obj, int type, void *data)
        if (obj->flags & REACHABLE)
                return 0;
        obj->flags |= REACHABLE;
        if (obj->flags & REACHABLE)
                return 0;
        obj->flags |= REACHABLE;

-       if (!obj->parsed) {
+       if (!(obj->flags & HAS_OBJ)) {

                if (parent && !has_sha1_file(obj->sha1)) {
                        printf("broken link from %7s %s\n",
                                 typename(parent->type), sha1_to_hex(parent->sha1));
                if (parent && !has_sha1_file(obj->sha1)) {
                        printf("broken link from %7s %s\n",
                                 typename(parent->type), sha1_to_hex(parent->sha1));


@@ -127,16 +128,13 @@ static int traverse_one_object(struct object *obj)
        struct tree *tree = NULL;
 
        if (obj->type == OBJ_TREE) {
        struct tree *tree = NULL;
 
        if (obj->type == OBJ_TREE) {

-               obj->parsed = 0;

                tree = (struct tree *)obj;
                if (parse_tree(tree) < 0)
                        return 1; /* error already displayed */
        }
        result = fsck_walk(obj, mark_object, obj);
                tree = (struct tree *)obj;
                if (parse_tree(tree) < 0)
                        return 1; /* error already displayed */
        }
        result = fsck_walk(obj, mark_object, obj);

-       if (tree) {
-               free(tree->buffer);
-               tree->buffer = NULL;
-       }
+       if (tree)
+               free_tree_buffer(tree);

        return result;
 }
 
        return result;
 }
 


@@ -178,7 +176,7 @@ static void check_reachable_object(struct object *obj)
         * except if it was in a pack-file and we didn't
         * do a full fsck
         */
         * except if it was in a pack-file and we didn't
         * do a full fsck
         */

-       if (!obj->parsed) {
+       if (!(obj->flags & HAS_OBJ)) {

                if (has_sha1_pack(obj->sha1))
                        return; /* it is in pack - forget about it */
                printf("missing %s %s\n", typename(obj->type), sha1_to_hex(obj->sha1));
                if (has_sha1_pack(obj->sha1))
                        return; /* it is in pack - forget about it */
                printf("missing %s %s\n", typename(obj->type), sha1_to_hex(obj->sha1));


@@ -306,8 +304,7 @@ static int fsck_obj(struct object *obj)
        if (obj->type == OBJ_TREE) {
                struct tree *item = (struct tree *) obj;
 
        if (obj->type == OBJ_TREE) {
                struct tree *item = (struct tree *) obj;
 

-               free(item->buffer);
-               item->buffer = NULL;
+               free_tree_buffer(item);

        }
 
        if (obj->type == OBJ_COMMIT) {
        }
 
        if (obj->type == OBJ_COMMIT) {


@@ -340,6 +337,7 @@ static int fsck_sha1(const unsigned char *sha1)
                return error("%s: object corrupt or missing",
                             sha1_to_hex(sha1));
        }
                return error("%s: object corrupt or missing",
                             sha1_to_hex(sha1));
        }

+       obj->flags |= HAS_OBJ;

        return fsck_obj(obj);
 }
 
        return fsck_obj(obj);
 }
 


@@ -352,6 +350,7 @@ static int fsck_obj_buffer(const unsigned char *sha1, enum object_type type,
                errors_found |= ERROR_OBJECT;
                return error("%s: object corrupt or missing", sha1_to_hex(sha1));
        }
                errors_found |= ERROR_OBJECT;
                return error("%s: object corrupt or missing", sha1_to_hex(sha1));
        }

+       obj->flags = HAS_OBJ;

        return fsck_obj(obj);
 }
 
        return fsck_obj(obj);
 }
 

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 79dfe47320e244c3110ce65d5b19fa87ebca3cb5..20cf284e869e2e37eded2c9bd990453461c75359 100644 (file)
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -765,6 +765,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
                        if (obj->type == OBJ_TREE) {
                                struct tree *item = (struct tree *) obj;
                                item->buffer = NULL;
                        if (obj->type == OBJ_TREE) {
                                struct tree *item = (struct tree *) obj;
                                item->buffer = NULL;

+                               obj->parsed = 0;

                        }
                        if (obj->type == OBJ_COMMIT) {
                                struct commit *commit = (struct commit *) obj;
                        }
                        if (obj->type == OBJ_COMMIT) {
                                struct commit *commit = (struct commit *) obj;

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 72a0af70c3dcd7d108887cd289bc7b3c606226aa..9f121cd587060fc61eb3f39d8f5418ddc225c498 100644 (file)
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -94,8 +94,7 @@ static int tree_is_complete(const unsigned char *sha1)
                        complete = 0;
                }
        }
                        complete = 0;
                }
        }

-       free(tree->buffer);
-       tree->buffer = NULL;
+       free_tree_buffer(tree);

 
        if (complete)
                tree->object.flags |= SEEN;
 
        if (complete)
                tree->object.flags |= SEEN;

diff --git a/http-push.c b/http-push.c
index 395a8cfc1055fb6febc9cee559d8943bb4d9e829..c13b44184dd94272331007ad75ed3716c12c9307 100644 (file)
--- a/http-push.c
+++ b/http-push.c
@@ -1330,8 +1330,7 @@ static struct object_list **process_tree(struct tree *tree,
                        break;
                }
 
                        break;
                }
 

-       free(tree->buffer);
-       tree->buffer = NULL;
+       free_tree_buffer(tree);

        return p;
 }
 
        return p;
 }
 

diff --git a/list-objects.c b/list-objects.c
index 3dd4a960190a1b0016b26dec9187692111b73e3f..c8c3463cad6db4145434ee08f6c3a494e26a8675 100644 (file)
--- 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);
                                     cb_data);
        }
        strbuf_setlen(base, baselen);

-       free(tree->buffer);
-       tree->buffer = NULL;
+       free_tree_buffer(tree);

 }
 
 static void mark_edge_parents_uninteresting(struct commit *commit,
 }
 
 static void mark_edge_parents_uninteresting(struct commit *commit,

diff --git a/reachable.c b/reachable.c
index e7e6a1e342200bbf4c37bd561e6a68222349418e..654a8c58d689daf43f2ba4c40e4fe54d31c7826a 100644 (file)
--- 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);
        }
                else
                        process_blob(lookup_blob(entry.sha1), p, &me, entry.path, cp);
        }

-       free(tree->buffer);
-       tree->buffer = NULL;
+       free_tree_buffer(tree);

 }
 
 static void process_tag(struct tag *tag, struct object_array *p,
 }
 
 static void process_tag(struct tag *tag, struct object_array *p,

diff --git a/revision.c b/revision.c
index a67b615bfc7001ac28e339db1d91ea46a1b125b5..2190267e9a462b1912b86256ea88abf40a37abed 100644 (file)
--- a/revision.c
+++ b/revision.c
@@ -134,8 +134,7 @@ void mark_tree_uninteresting(struct tree *tree)
         * We don't care about the tree any more
         * after it has been marked uninteresting.
         */
         * We don't care about the tree any more
         * after it has been marked uninteresting.
         */

-       free(tree->buffer);
-       tree->buffer = NULL;
+       free_tree_buffer(tree);

 }
 
 void mark_parents_uninteresting(struct commit *commit)
 }
 
 void mark_parents_uninteresting(struct commit *commit)

diff --git a/tree.c b/tree.c
index 62fed632d8a1ade389dd645b5887155c21ae25ba..1cbf60ea2377e588a63bd7980163b9a24daf8bac 100644 (file)
--- a/tree.c
+++ b/tree.c
@@ -225,6 +225,14 @@ int parse_tree(struct tree *item)
        return parse_tree_buffer(item, buffer, size);
 }
 
        return parse_tree_buffer(item, buffer, size);
 }
 

+void free_tree_buffer(struct tree *tree)
+{
+       free(tree->buffer);
+       tree->buffer = NULL;
+       tree->size = 0;
+       tree->object.parsed = 0;
+}
+

 struct tree *parse_tree_indirect(const unsigned char *sha1)
 {
        struct object *obj = parse_object(sha1);
 struct tree *parse_tree_indirect(const unsigned char *sha1)
 {
        struct object *obj = parse_object(sha1);

diff --git a/tree.h b/tree.h
index 69bcb5e0ec27de6699e349b8dcec26f1cbc4e741..601ab9c50d166d51e20322a5012b106514f90a71 100644 (file)
--- a/tree.h
+++ b/tree.h
@@ -16,6 +16,7 @@ struct tree *lookup_tree(const unsigned char *sha1);
 int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size);
 
 int parse_tree(struct tree *tree);
 int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size);
 
 int parse_tree(struct tree *tree);

+void free_tree_buffer(struct tree *tree);

 
 /* Parses and returns the tree in the given ent, chasing tags and commits. */
 struct tree *parse_tree_indirect(const unsigned char *sha1);
 
 /* Parses and returns the tree in the given ent, chasing tags and commits. */
 struct tree *parse_tree_indirect(const unsigned char *sha1);

diff --git a/walker.c b/walker.c
index be389dc9bf5161c31be29e3a72264fd6120a0bbc..633596e06fcaa1154980f95c858c61379c968d49 100644 (file)
--- 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;
        }
                if (!obj || process(walker, obj))
                        return -1;
        }

-       free(tree->buffer);
-       tree->buffer = NULL;
-       tree->size = 0;
-       tree->object.parsed = 0;
+       free_tree_buffer(tree);

        return 0;
 }
 
        return 0;
 }