object_array_entry: fix memory handling of the name field

Previously, the memory management of the object_array_entry::name
field was inconsistent and undocumented.  object_array_entries are
ultimately created by a single function, add_object_array_with_mode(),
which has an argument "const char *name".  This function used to
simply set the name field to reference the string pointed to by the
name parameter, and nobody on the object_array side ever freed the
memory.  Thus, it assumed that the memory for the name field would be
managed by the caller, and that the lifetime of that string would be
at least as long as the lifetime of the object_array_entry.  But
callers were inconsistent:

* Some passed pointers to constant strings or argv entries, which was
OK.

* Some passed pointers to newly-allocated memory, but didn't arrange
for the memory ever to be freed.

* Some passed the return value of sha1_to_hex(), which is a pointer to
a statically-allocated buffer that can be overwritten at any time.

* Some passed pointers to refnames that they received from a
for_each_ref()-type iteration, but the lifetimes of such refnames is
not guaranteed by the refs API.

Bring consistency to this mess by changing object_array to make its
own copy for the object_array_entry::name field and free this memory
when an object_array_entry is deleted from the array.

Many callers were passing the empty string as the name parameter, so
as a performance optimization, treat the empty string specially.
Instead of making a copy, store a pointer to a statically-allocated
empty string to object_array_entry::name.  When deleting such an
entry, skip the free().

Change the callers that were already passing copies to
add_object_array_with_mode() to either skip the copy, or (if the
memory needed to be allocated anyway) freeing the memory itself.

A part of this commit effectively reverts

70d26c6e76 read_revisions_from_stdin: make copies for handle_revision_arg

because the copying introduced by that commit (which is still
necessary) is now done at a deeper level.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/bundle.c b/bundle.c
index 4b0e5cd51b82f916c26d3776f81f22cdb3045fa6..3d6431137336f9748247dfad625555d0a546b26b 100644 (file)
--- a/bundle.c
+++ b/bundle.c
@@ -281,7 +281,7 @@ int create_bundle(struct bundle_header *header, const char *path,
                        if (!get_sha1_hex(buf.buf + 1, sha1)) {
                                struct object *object = parse_object_or_die(sha1, buf.buf);
                                object->flags |= UNINTERESTING;
-                               add_pending_object(&revs, object, xstrdup(buf.buf));
+                               add_pending_object(&revs, object, buf.buf);
                        }
                } else if (!get_sha1_hex(buf.buf, sha1)) {
                        struct object *object = parse_object_or_die(sha1, buf.buf);

diff --git a/object.c b/object.c
index 10b534946126f7d7e640f9634ebd2991373eae32..243d694a58334cc45df6f38878d094ed419188ce 100644 (file)
--- a/object.c
+++ b/object.c
@@ -260,11 +260,18 @@ void add_object_array(struct object *obj, const char *name, struct object_array
        add_object_array_with_mode(obj, name, array, S_IFINVALID);
 }
 
+/*
+ * A zero-length string to which object_array_entry::name can be
+ * initialized without requiring a malloc/free.
+ */
+static char object_array_slopbuf[1];
+
 void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)
 {
        unsigned nr = array->nr;
        unsigned alloc = array->alloc;
        struct object_array_entry *objects = array->objects;
+       struct object_array_entry *entry;
 
        if (nr >= alloc) {
                alloc = (alloc + 32) * 2;
@@ -272,9 +279,16 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj
                array->alloc = alloc;
                array->objects = objects;
        }
-       objects[nr].item = obj;
-       objects[nr].name = name;
-       objects[nr].mode = mode;
+       entry = &objects[nr];
+       entry->item = obj;
+       if (!name)
+               entry->name = NULL;
+       else if (!*name)
+               /* Use our own empty string instead of allocating one: */
+               entry->name = object_array_slopbuf;
+       else
+               entry->name = xstrdup(name);
+       entry->mode = mode;
        array->nr = ++nr;
 }
 
@@ -289,6 +303,9 @@ void object_array_filter(struct object_array *array,
                        if (src != dst)
                                objects[dst] = objects[src];
                        dst++;
+               } else {
+                       if (objects[src].name != object_array_slopbuf)
+                               free(objects[src].name);
                }
        }
        array->nr = dst;
@@ -319,6 +336,9 @@ void object_array_remove_duplicates(struct object_array *array)
                        if (src != array->nr)
                                objects[array->nr] = objects[src];
                        array->nr++;
+               } else {
+                       if (objects[src].name != object_array_slopbuf)
+                               free(objects[src].name);
                }
        }
 }

diff --git a/object.h b/object.h
index 6c1c27fba697f436c3f22d1d11b9babd5d42f493..2ff68c52dd48842a188eb8f0c9b1e12ff27fae37 100644 (file)
--- a/object.h
+++ b/object.h
@@ -11,7 +11,13 @@ struct object_array {
        unsigned int alloc;
        struct object_array_entry {
                struct object *item;
-               const char *name;
+               /*
+                * name or NULL.  If non-NULL, the memory pointed to
+                * is owned by this object *except* if it points at
+                * object_array_slopbuf, which is a static copy of the
+                * empty string.
+                */
+               char *name;
                unsigned mode;
        } *objects;
 };

diff --git a/revision.c b/revision.c
index be73cb46c95e87dd67ac72eb72d575bcd5fc7437..4aeda334dbb7693563800da6da123d0094d72963 100644 (file)
--- a/revision.c
+++ b/revision.c
@@ -88,7 +88,9 @@ void add_object(struct object *obj,
                struct name_path *path,
                const char *name)
 {
-       add_object_array(obj, path_name(path, name), p);
+       char *pn = path_name(path, name);
+       add_object_array(obj, pn, p);
+       free(pn);
 }
 
 static void mark_blob_uninteresting(struct blob *blob)
@@ -1288,7 +1290,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,
                        }
                        die("options not supported in --stdin mode");
                }
-               if (handle_revision_arg(xstrdup(sb.buf), revs, 0,
+               if (handle_revision_arg(sb.buf, revs, 0,
                                        REVARG_CANNOT_BE_FILENAME))
                        die("bad revision '%s'", sb.buf);
        }