split_symref_update() receives a string-pointer `referent` and adds it
to the list of `affected_refnames`. The list simply holds on to the
pointers it is given, it does not copy the strings and it never frees
them. The `referent` string in split_symref_update() belongs to a string
buffer in the caller. After we return, the string will be leaked.
In the next patch, we want to properly release the string buffer in the
caller, but we can't safely do so until we've made sure that
`affected_refnames` will not be holding on to a pointer to the string.
We could configure the list to handle its own resources, but it would
mean some alloc/free-churning. The list is already handling other
strings (through other code paths) which we do not need to worry about,
and we'd be memory-churning those strings too, completely unnecessary.
Observe that split_symref_update() creates a `new_update`-object and
that `new_update->refname` is then a copy of `referent`. The difference
is, this copy will be freed, and it will be freed *after*
`affected_refnames` has been cleared.
Rearrange the handling of `referent`, so that we don't add it directly
to `affected_refnames`. Instead, first just check whether `referent`
exists in the string list, and later add `new_update->refname`.
Helped-by: Michael Haggerty <mhagger@xxxxxxxxxxxx>
Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx>
---
Thanks Junio and Michael for your comments on the first version. This
first patch is now completely different and much much better (thanks
Michael!). The commit message should also be better (sorry Junio...).
The second one has a new commit message, but the diff is the same.
Martin
refs/files-backend.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5cca55510..bdb0e22e5 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2140,13 +2140,12 @@ static int split_symref_update(struct files_ref_store *refs,
/*
* First make sure that referent is not already in the
- * transaction. This insertion is O(N) in the transaction
+ * transaction. This check is O(N) in the transaction
* size, but it happens at most once per symref in a
* transaction.
*/
- item = string_list_insert(affected_refnames, referent);
- if (item->util) {
- /* An entry already existed */
+ if (string_list_has_string(affected_refnames, referent)) {
+ /* An entry already exists */
strbuf_addf(err,
"multiple updates for '%s' (including one "
"via symref '%s') are not allowed",
@@ -2181,6 +2180,15 @@ static int split_symref_update(struct files_ref_store *refs,
update->flags |= REF_LOG_ONLY | REF_NODEREF;
update->flags &= ~REF_HAVE_OLD;
+ /*
+ * Add the referent. This insertion is O(N) in the transaction
+ * size, but it happens at most once per symref in a
+ * transaction. Make sure to add new_update->refname, which will
+ * be valid as long as affected_refnames is in use, and NOT
+ * referent, which might soon be freed by our caller.
+ */
+ item = string_list_insert(affected_refnames, new_update->refname);
+ assert(!item->util);
item->util = new_update;
return 0;
--
2.14.1.151.g45c1275a3.dirty
