Jeff King <peff@xxxxxxxx> writes:
> On Thu, May 15, 2025 at 02:55:36PM -0400, Jeff King wrote:
>
>> On Thu, May 15, 2025 at 04:07:28PM +0200, Karthik Nayak wrote:
>>
>> > +failure:
>> > + for (cmd = commands; cmd; cmd = cmd->next) {
>> > + if (reported_error)
>> > + cmd->error_string = reported_error;
>> > + else if (strmap_contains(&failed_refs, cmd->ref_name))
>> > + cmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));
>> > }
>
> BTW, one other funny thing about this code: we duplicate the strings
> that we assign to cmd->error_string. But no other call path does so. So
> now we have allocated memory, but nobody can ever free() it because
> often it is not allocated!
>
> I'm surprised LSan does not report a leak here, so I might be missing
> something.
>
Yeah, indeed that is a leak and surprising that this wasn't caught by
LSan.
> It looks like there is some magic in the struct here to handle this
> like:
>
> cmd->error_string = cmd->error_string_owned = xstrdup(...);
>
> which would solve it. But I wonder: do these need to be allocated at
> all? We are pulling out strings owned by the strmap, which will free
> them. So as-is, yes, we need to make our own copies.
>
> But does the strmap need to own them in the first place? It is taking
> the values from ref_transaction_error_msg(). That returns an allocated
> string, but it doesn't need to. It could just return the string literals
> directly, which are valid for the life of the program. That would save
> other callers from having to call free(), though it does mean we have to
> cast away the constness when putting them into the strmap (since it
> accepts a non-const void pointer). Something like:
>
Thanks Jeff, this does make a lot of sense, I think with Junio's
suggestion in the first patch, making `ref_transaction_error_msg()`
return a 'const char *' solves this issue too, we no longer will hold
allocated strings here and as such no longer have to worry about
allocation/freeing of memory. Much cleaner.
In the end it looks similar to you patch below!
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 4a3c46eca7..cf0bcf1ad0 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1665,10 +1665,9 @@ static void ref_transaction_rejection_handler(const char *refname,
> "branches"), data->remote_name);
> data->conflict_msg_shown = 1;
> } else {
> - char *reason = ref_transaction_error_msg(err);
> + const char *reason = ref_transaction_error_msg(err);
>
> error(_("fetching ref %s failed: %s"), refname, reason);
> - free(reason);
> }
>
> *data->retcode = 1;
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index bd0fb729ff..4cef2ddd3d 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1855,7 +1855,7 @@ static void ref_transaction_rejection_handler(const char *refname,
> {
> struct strmap *failed_refs = cb_data;
>
> - strmap_put(failed_refs, refname, ref_transaction_error_msg(err));
> + strmap_put(failed_refs, refname, (char *)ref_transaction_error_msg(err));
> }
>
> static void execute_commands_non_atomic(struct command *commands,
> @@ -1897,15 +1897,16 @@ static void execute_commands_non_atomic(struct command *commands,
>
> failure:
> for (cmd = commands; cmd; cmd = cmd->next) {
> + const char *reason;
> if (reported_error)
> cmd->error_string = reported_error;
> - else if (strmap_contains(&failed_refs, cmd->ref_name))
> - cmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));
> + else if ((reason = strmap_get(&failed_refs, cmd->ref_name)))
> + cmd->error_string = reason;
> }
>
> cleanup:
> ref_transaction_free(transaction);
> - strmap_clear(&failed_refs, 1);
> + strmap_clear(&failed_refs, 0);
> strbuf_release(&err);
> }
>
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 09b99143bf..1e6131e04a 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -575,15 +575,14 @@ static void print_rejected_refs(const char *refname,
> void *cb_data UNUSED)
> {
> struct strbuf sb = STRBUF_INIT;
> - char *reason = ref_transaction_error_msg(err);
> + const char *reason = ref_transaction_error_msg(err);
>
> strbuf_addf(&sb, "rejected %s %s %s %s\n", refname,
> new_oid ? oid_to_hex(new_oid) : new_target,
> old_oid ? oid_to_hex(old_oid) : old_target,
> reason);
>
> fwrite(sb.buf, sb.len, 1, stdout);
> - free(reason);
> strbuf_release(&sb);
> }
>
> diff --git a/refs.c b/refs.c
> index 351ed52deb..953f83bb52 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3315,7 +3315,7 @@ int ref_update_expects_existing_old_ref(struct ref_update *update)
> (!is_null_oid(&update->old_oid) || update->old_target);
> }
>
> -char *ref_transaction_error_msg(enum ref_transaction_error err)
> +const char *ref_transaction_error_msg(enum ref_transaction_error err)
> {
> const char *reason = "";
>
> @@ -3342,5 +3342,5 @@ char *ref_transaction_error_msg(enum ref_transaction_error err)
> reason = "unkown failure";
> }
>
> - return xstrdup(reason);
> + return reason;
> }
> diff --git a/refs.h b/refs.h
> index a0b2e3c43d..2d58af3d88 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -910,7 +910,7 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio
> /*
> * Translate errors to human readable error messages.
> */
> -char *ref_transaction_error_msg(enum ref_transaction_error err);
> +const char *ref_transaction_error_msg(enum ref_transaction_error err);
>
> /*
> * Free `*transaction` and all associated data.
>
> -Peff
Attachment:
signature.asc
Description: PGP signature
