Re: [PATCH v2 6/6] update-ref: add support for 'symref-update' command
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- May 29, 2024, 08:29 UTC
- Message-ID
- <CAOLa=ZSMox99eZ=QmkucN+M+SxzFkQ3ivEzOfa-SZ=kKtEtcMw@mail.gmail.com>
- In-Reply-To
- <xmqqplt9h4yq.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 35 quoted lines
> Karthik Nayak <karthik.188@gmail.com> 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.
Show 18 quoted lines
> 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.