From c299468bd7bd5a6a8ab7350c260bb8d548cd1e1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sat, 9 Sep 2017 08:57:15 +0200 Subject: [PATCH 1/4] refs/files-backend: add longer-scoped copy of string to list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Michael Haggerty Signed-off-by: Martin Ågren Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- 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 0404f2c233..baceef3b3c 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2634,13 +2634,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", @@ -2675,6 +2674,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; From 851e1fbd01250f56a6e479e1addada220a56e1f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sat, 9 Sep 2017 08:57:16 +0200 Subject: [PATCH 2/4] refs/files-backend: fix memory leak in lock_ref_for_update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After the previous patch, none of the functions we call hold on to `referent.buf`, so we can safely release the string buffer before returning. Reviewed-by: Michael Haggerty Signed-off-by: Martin Ågren Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index baceef3b3c..3d63639665 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2756,7 +2756,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, struct strbuf referent = STRBUF_INIT; int mustexist = (update->flags & REF_HAVE_OLD) && !is_null_oid(&update->old_oid); - int ret; + int ret = 0; struct ref_lock *lock; files_assert_main_repository(refs, "lock_ref_for_update"); @@ -2768,7 +2768,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, ret = split_head_update(update, transaction, head_ref, affected_refnames, err); if (ret) - return ret; + goto out; } ret = lock_raw_ref(refs, update->refname, mustexist, @@ -2782,7 +2782,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, strbuf_addf(err, "cannot lock ref '%s': %s", original_update_refname(update), reason); free(reason); - return ret; + goto out; } update->backend_data = lock; @@ -2801,10 +2801,12 @@ static int lock_ref_for_update(struct files_ref_store *refs, strbuf_addf(err, "cannot lock ref '%s': " "error reading reference", original_update_refname(update)); - return -1; + ret = -1; + goto out; } } else if (check_old_oid(update, &lock->old_oid, err)) { - return TRANSACTION_GENERIC_ERROR; + ret = TRANSACTION_GENERIC_ERROR; + goto out; } } else { /* @@ -2818,13 +2820,15 @@ static int lock_ref_for_update(struct files_ref_store *refs, referent.buf, transaction, affected_refnames, err); if (ret) - return ret; + goto out; } } else { struct ref_update *parent_update; - if (check_old_oid(update, &lock->old_oid, err)) - return TRANSACTION_GENERIC_ERROR; + if (check_old_oid(update, &lock->old_oid, err)) { + ret = TRANSACTION_GENERIC_ERROR; + goto out; + } /* * If this update is happening indirectly because of a @@ -2861,7 +2865,8 @@ static int lock_ref_for_update(struct files_ref_store *refs, "cannot update ref '%s': %s", update->refname, write_err); free(write_err); - return TRANSACTION_GENERIC_ERROR; + ret = TRANSACTION_GENERIC_ERROR; + goto out; } else { update->flags |= REF_NEEDS_COMMIT; } @@ -2875,10 +2880,14 @@ static int lock_ref_for_update(struct files_ref_store *refs, if (close_ref(lock)) { strbuf_addf(err, "couldn't close '%s.lock'", update->refname); - return TRANSACTION_GENERIC_ERROR; + ret = TRANSACTION_GENERIC_ERROR; + goto out; } } - return 0; + +out: + strbuf_release(&referent); + return ret; } /* From 3f5ef95b5e0cc0cbff06f747ba056e132a71033a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sat, 9 Sep 2017 08:57:17 +0200 Subject: [PATCH 3/4] refs/files-backend: correct return value in lock_ref_for_update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In one code path we return a literal -1 and not a symbolic constant. The value -1 would be interpreted as TRANSACTION_NAME_CONFLICT, which is wrong. Use TRANSACTION_GENERIC_ERROR instead (that is the only other return value we have to choose from). Noticed-by: Michael Haggerty Reviewed-by: Michael Haggerty Signed-off-by: Martin Ågren Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 3d63639665..03df002759 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2801,7 +2801,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, strbuf_addf(err, "cannot lock ref '%s': " "error reading reference", original_update_refname(update)); - ret = -1; + ret = TRANSACTION_GENERIC_ERROR; goto out; } } else if (check_old_oid(update, &lock->old_oid, err)) { From 276d0e35c0c9cadaf38247f7a9ff716e293b2acb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sat, 9 Sep 2017 08:57:18 +0200 Subject: [PATCH 4/4] refs/files-backend: add `refname`, not "HEAD", to list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An earlier patch rewrote `split_symref_update()` to add a copy of a string to a string list instead of adding the original string. That was so that the original string could be freed in a later patch, but it is also conceptually cleaner, since now all calls to `string_list_insert()` and `string_list_append()` add `update->refname`. --- Except a literal "HEAD" is added in `split_head_update()`. Restructure `split_head_update()` in the same way as the earlier patch did for `split_symref_update()`. This does not correct any practical problem, but makes things conceptually cleaner. The downside is a call to `string_list_has_string()`, which should be relatively cheap. Signed-off-by: Jeff King Signed-off-by: Martin Ågren Reviewed-by: Jeff King Signed-off-by: Junio C Hamano --- refs/files-backend.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 03df002759..926f87b945 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2589,11 +2589,10 @@ static int split_head_update(struct ref_update *update, /* * First make sure that HEAD 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 transaction. */ - item = string_list_insert(affected_refnames, "HEAD"); - if (item->util) { + if (string_list_has_string(affected_refnames, "HEAD")) { /* An entry already existed */ strbuf_addf(err, "multiple updates for 'HEAD' (including one " @@ -2608,6 +2607,14 @@ static int split_head_update(struct ref_update *update, update->new_oid.hash, update->old_oid.hash, update->msg); + /* + * Add "HEAD". This insertion is O(N) in the transaction + * size, but it happens at most once per transaction. + * Add new_update->refname instead of a literal "HEAD". + */ + if (strcmp(new_update->refname, "HEAD")) + BUG("%s unexpectedly not 'HEAD'", new_update->refname); + item = string_list_insert(affected_refnames, new_update->refname); item->util = new_update; return 0;