Junio C Hamano [off-list ref] writes:

Karthik Nayak [off-list ref] writes:

quoted

+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);

quoted

+	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() ...

quoted

+		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 ...

quoted

+		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.

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.