[PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- Aug 29, 2017, 17:18 UTC
- Message-ID
- <4b4c0d178ad2216eecbc49fb6f54dd8a1d1ac119.1504024261.git.martin.agren@gmail.com>
- In-Reply-To
- <CAN0heSqn59sFF3A-eQ593G+ZDWnO9pKM5F=sgiSQk+prUr-nSQ@mail.gmail.com>
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 does not ever free 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 through ref_transaction_add_update(), after which `new_update->refname` is 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@alum.mit.edu> Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- v3: "O(lg N)" in comment; if-BUG() instead of assert()
refs/files-backend.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c index fccbc24ac..73615d869 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2144,13 +2144,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(lg 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", @@ -2185,6 +2184,17 @@ 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); + if (item->util) + BUG("%s unexpectedly found in affected_refnames", + new_update->refname); item->util = new_update; return 0;
-- 2.14.1.151.g45c1275a3.dirty