git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 15/25] object_array_entry: fix memory handling of the name field

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
May 25, 2013, 09:08 UTC
Message-ID
<1369472904-12875-16-git-send-email-mhagger@alum.mit.edu>
In-Reply-To
<1369472904-12875-1-git-send-email-mhagger@alum.mit.edu>

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>
---
 bundle.c   |  2 +-
 object.c   | 26 +++++++++++++++++++++++---
 object.h   |  8 +++++++-
 revision.c |  6 ++++--
 4 files changed, 35 insertions(+), 7 deletions(-)
diff --git a/bundle.c b/bundle.c
index 4b0e5cd..3d64311 100644
--- 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 10b5349..e4ff714 100644
--- 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.
+ */
+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 6c1c27f..2ff68c5 100644
--- 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 be73cb4..4aeda33 100644
--- 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);
 	}

-- 
1.8.2.3
Previous: Michael HaggertyNext: Junio C Hamano
Message 19 of 40 in “Remove assumptions about each_ref_fn arg lifetimes”
  1. 00/25 Remove assumptions about each_ref_fn arg lifetimesMichael Haggerty, May 25, 2013
  2. 01/25 describe: make own copy of refnameMichael Haggerty, May 25, 2013
  3. 02/25 fetch: make own copies of refnamesMichael Haggerty, May 25, 2013
  4. 03/25 add_rev_cmdline(): make a copy of the name argumentMichael Haggerty, May 25, 2013
  5. 04/25 builtin_diff_tree(): make it obvious that function wants two entriesMichael Haggerty, May 25, 2013
  6. 05/25 cmd_diff(): use an object_array for holding treesMichael Haggerty, May 25, 2013
  7. 06/25 cmd_diff(): rename local variable "list" -> "entry"Michael Haggerty, May 25, 2013
  8. 07/25 cmd_diff(): make it obvious which cases are exclusive of each otherMichael Haggerty, May 25, 2013
  9. 08/25 revision: split some overly-long linesMichael Haggerty, May 25, 2013
  10. 09/25 object_array: add function object_array_filter()Michael Haggerty, May 25, 2013
  11. 10/25 revision: use object_array_filter() in implementation of gc_boundary()Michael Haggerty, May 25, 2013
  12. 11/25 object_array_remove_duplicates(): rewrite to reduce copyingMichael Haggerty, May 25, 2013
  13. Junio C HamanoMay 29, 2013
  14. Michael HaggertyMay 30, 2013
  15. Junio C HamanoJun 2, 2013
  16. 12/25 fsck: don't put a void*-shaped peg in a char*-shaped holeMichael Haggerty, May 25, 2013
  17. 13/25 find_first_merges(): initialize merges variable using initializerMichael Haggerty, May 25, 2013
  18. 14/25 find_first_merges(): remove unnecessary codeMichael Haggerty, May 25, 2013
  19. 15/25 object_array_entry: fix memory handling of the name fieldMichael Haggerty, May 25, 2013
  20. Junio C HamanoMay 29, 2013
  21. 16/25 do_fetch(): reduce scope of peer_itemMichael Haggerty, May 25, 2013
  22. 17/25 do_fetch(): clean up existing_refs before exitingMichael Haggerty, May 25, 2013
  23. 18/25 add_existing(): do not retain a reference to sha1Michael Haggerty, May 25, 2013
  24. 19/25 show_head_ref(): do not shadow name of argumentMichael Haggerty, May 25, 2013
  25. 20/25 show_head_ref(): rename first parameter to "refname"Michael Haggerty, May 25, 2013
  26. 21/25 string_list_add_one_ref(): rename first parameter to "refname"Michael Haggerty, May 25, 2013
  27. 22/25 string_list_add_refs_by_glob(): add a comment about memory managementMichael Haggerty, May 25, 2013
  28. Thomas RastMay 29, 2013
  29. Michael HaggertyMay 30, 2013
  30. 22/25 fixup! string_list_add_refs_by_glob(): add a comment about memory managementMichael Haggerty, May 30, 2013
  31. Junio C HamanoJun 3, 2013
  32. 23/25 exclude_existing(): set existing_refs.strdup_stringsMichael Haggerty, May 25, 2013
  33. 24/25 register_ref(): make a copy of the bad reference SHA-1Michael Haggerty, May 25, 2013
  34. Junio C HamanoMay 29, 2013
  35. Michael HaggertyMay 30, 2013
  36. Philip OakleyMay 30, 2013
  37. 25/25 refs: document the lifetime of the args passed to each_ref_fnMichael Haggerty, May 25, 2013
  38. Junio C HamanoMay 29, 2013
  39. Thomas RastMay 29, 2013
  40. Michael HaggertyMay 30, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.