Skip to content

Commit 31faeb2

Browse files
mhaggergitster
authored andcommitted
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 70d26c6 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 <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 5de0c01 commit 31faeb2

File tree

4 files changed

+35
-7
lines changed

4 files changed

+35
-7
lines changed

bundle.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,7 @@ int create_bundle(struct bundle_header *header, const char *path,
281281
if (!get_sha1_hex(buf.buf + 1, sha1)) {
282282
struct object *object = parse_object_or_die(sha1, buf.buf);
283283
object->flags |= UNINTERESTING;
284-
add_pending_object(&revs, object, xstrdup(buf.buf));
284+
add_pending_object(&revs, object, buf.buf);
285285
}
286286
} else if (!get_sha1_hex(buf.buf, sha1)) {
287287
struct object *object = parse_object_or_die(sha1, buf.buf);

object.c

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -260,21 +260,35 @@ void add_object_array(struct object *obj, const char *name, struct object_array
260260
add_object_array_with_mode(obj, name, array, S_IFINVALID);
261261
}
262262

263+
/*
264+
* A zero-length string to which object_array_entry::name can be
265+
* initialized without requiring a malloc/free.
266+
*/
267+
static char object_array_slopbuf[1];
268+
263269
void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode)
264270
{
265271
unsigned nr = array->nr;
266272
unsigned alloc = array->alloc;
267273
struct object_array_entry *objects = array->objects;
274+
struct object_array_entry *entry;
268275

269276
if (nr >= alloc) {
270277
alloc = (alloc + 32) * 2;
271278
objects = xrealloc(objects, alloc * sizeof(*objects));
272279
array->alloc = alloc;
273280
array->objects = objects;
274281
}
275-
objects[nr].item = obj;
276-
objects[nr].name = name;
277-
objects[nr].mode = mode;
282+
entry = &objects[nr];
283+
entry->item = obj;
284+
if (!name)
285+
entry->name = NULL;
286+
else if (!*name)
287+
/* Use our own empty string instead of allocating one: */
288+
entry->name = object_array_slopbuf;
289+
else
290+
entry->name = xstrdup(name);
291+
entry->mode = mode;
278292
array->nr = ++nr;
279293
}
280294

@@ -289,6 +303,9 @@ void object_array_filter(struct object_array *array,
289303
if (src != dst)
290304
objects[dst] = objects[src];
291305
dst++;
306+
} else {
307+
if (objects[src].name != object_array_slopbuf)
308+
free(objects[src].name);
292309
}
293310
}
294311
array->nr = dst;
@@ -319,6 +336,9 @@ void object_array_remove_duplicates(struct object_array *array)
319336
if (src != array->nr)
320337
objects[array->nr] = objects[src];
321338
array->nr++;
339+
} else {
340+
if (objects[src].name != object_array_slopbuf)
341+
free(objects[src].name);
322342
}
323343
}
324344
}

object.h

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,13 @@ struct object_array {
1111
unsigned int alloc;
1212
struct object_array_entry {
1313
struct object *item;
14-
const char *name;
14+
/*
15+
* name or NULL. If non-NULL, the memory pointed to
16+
* is owned by this object *except* if it points at
17+
* object_array_slopbuf, which is a static copy of the
18+
* empty string.
19+
*/
20+
char *name;
1521
unsigned mode;
1622
} *objects;
1723
};

revision.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,9 @@ void add_object(struct object *obj,
8888
struct name_path *path,
8989
const char *name)
9090
{
91-
add_object_array(obj, path_name(path, name), p);
91+
char *pn = path_name(path, name);
92+
add_object_array(obj, pn, p);
93+
free(pn);
9294
}
9395

9496
static void mark_blob_uninteresting(struct blob *blob)
@@ -1288,7 +1290,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,
12881290
}
12891291
die("options not supported in --stdin mode");
12901292
}
1291-
if (handle_revision_arg(xstrdup(sb.buf), revs, 0,
1293+
if (handle_revision_arg(sb.buf, revs, 0,
12921294
REVARG_CANNOT_BE_FILENAME))
12931295
die("bad revision '%s'", sb.buf);
12941296
}

0 commit comments

Comments
 (0)