Junio C Hamano <gitster@xxxxxxxxx> writes:
> Karthik Nayak <karthik.188@xxxxxxxxx> writes:
>
>> +static void parse_cmd_symref_update(struct ref_transaction *transaction,
>> + const char *next, const char *end)
>> +{
>> + char *refname, *new_target, *old_arg;
>> + char *old_target = NULL;
>> + ...
>> + old_arg = parse_next_arg(&next);
>
>> + if (old_arg) {
>> + old_target = parse_next_arg(&next);
>
> Now we have an allocated memory we are responsible for freeing in
> old_target, obtained from parse_next_arg() ...
>
>> + if (!old_target)
>> + die("symref-update %s: expected old value", refname);
>
> ... and here we know it is not NULL. We use it to grab the object
> name ...
>
>> + if (!strcmp(old_arg, "oid")) {
>> + if (repo_get_oid(the_repository, old_target, &old_oid))
>> + die("symref-update %s: invalid oid: %s", refname, old_target);
>> +
>> + old_target = NULL;
>
> ... and then we overwritten the variable, losing the last reference
> to the piece of memory without freeing.
>
> Perhaps squashing this in is sufficient to plug this leak, but there
> probably are other new leaks around this code. I ran out of time so
> I'll let you take care of the rest ;-)
>
Yeah this indeed was missed, I think having the
`TEST_PASSES_SANITIZE_LEAK` flag set, helped catch this leak and another
around strbuf.
> Thanks.
>
> builtin/update-ref.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git c/builtin/update-ref.c w/builtin/update-ref.c
> index 76d20ca0f1..7d2a419230 100644
> --- c/builtin/update-ref.c
> +++ w/builtin/update-ref.c
> @@ -297,7 +297,7 @@ static void parse_cmd_symref_update(struct ref_transaction *transaction,
>
> if (!strcmp(old_arg, "oid") &&
> !repo_get_oid(the_repository, old_target, &old_oid)) {
> - old_target = NULL;
> + FREE_AND_NULL(old_target);
> have_old = 1;
> } else if (strcmp(old_arg, "ref"))
> die("symref-update %s: invalid arg '%s' for old value", refname, old_arg);
This definitely works, but keeping it consistent with the rest of the
code in this file, I think we can do:
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index bda37c161d..82f461d6f8 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -298,7 +300,6 @@ static void parse_cmd_symref_update(struct
ref_transaction *transaction,
if (repo_get_oid(the_repository, old_target, &old_oid))
die("symref-update %s: invalid oid: %s", refname, old_target);
- old_target = NULL;
have_old_oid = 1;
} else if (!strcmp(old_arg, "ref")) {
if (check_refname_format(old_target, REFNAME_ALLOW_ONELEVEL))
@@ -313,7 +314,8 @@ static void parse_cmd_symref_update(struct
ref_transaction *transaction,
if (ref_transaction_update(transaction, refname, NULL,
have_old_oid ? &old_oid : NULL,
- new_target, old_target,
+ new_target,
+ have_old_oid ? NULL : old_target,
update_flags | create_reflog_flag,
msg, &err))
die("%s", err.buf);
This works, since we anyways do a `free(old_target)` at the end of this
function.
Attachment:
signature.asc
Description: PGP signature
