From: Derrick Stolee <stolee@gmail.com>
Date: Mon, 31 Aug 2026 12:41:44 GMT
Subject: Re: [PATCH v2 3/7] trace2: remove use of xstrdup()
Message-ID: <2eadc838-9d47-442d-a94a-efc570624489@gmail.com>
In-Reply-To: <CABPp-BH1TeDTeqddZw+cvzou+3PRgw+HNpYF2JnhMTSBp9qfbQ@mail.gmail.com>

On 8/25/2026 6:14 PM, Elijah Newren wrote:
> On Tue, Aug 25, 2026 at 11:58 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
> [...]
>> For full defense in depth, we remove the xstrdup() calls from
>> trace2/tr2_sysenv.c.
>>
>> First, in tr2_sysenv_cb(), we need to handle a failed assignment of the
>> value with a negative return to halt the config parsing loop.
>>
> [...]
>> --- a/trace2/tr2_sysenv.c
>> +++ b/trace2/tr2_sysenv.c
>> @@ -74,7 +74,9 @@ static int tr2_sysenv_cb(const char *key, const char *value,
>>                         if (!value)
>>                                 return config_error_nonbool(key);
>>                         free(tr2_sysenv_settings[k].value);
>> -                       tr2_sysenv_settings[k].value = xstrdup(value);
>> +                       tr2_sysenv_settings[k].value = strdup(value);
>> +                       if (!tr2_sysenv_settings[k].value)
>> +                               return -1;
> 
> I'm not sure if this matters, but I think the call sequence from
> config.c to this function is:
> 
>   read_very_early_config ->
>     config_with_options ->
>       git_config_from_file_with_options ->
>         do_config_from_file ->
>           do_config_from ->
>             git_parse_source ->
>               get_value ->
>                 git_config_include ->
>                   tr2_sysenv_cb
> 
> and the -1 unwinds back to git_parse_source, which breaks, formats an
> error message, and calls die:
> 
>    error_msg = xstrfmt(_("bad config line %d in file %s")...)
>    die("%s", error_msg)

Thanks for the careful read! It's particularly important that we
don't suggest that the config value is bad because we couldn't
allocate memory.

> Am I reading this right?  If so, the -1 actually triggers a die as
> well -- unless the allocation in xstrfmt manages to kill it first.
> This isn't a regression (the old xstrdup() also died) and the die
> isn't inside the trace functions, but the commit message might read as
> promising more than it delivers.

Yes, I believe you are correct. We should return 0 to terminate
early without a failure.

That said, I think that the die() in the config code will remain a
"safe" place to die(), as we won't re-trigger this config-parsing
code during any tracing of that die() message. But it's best to be
safe and have the tracing continue to be "best effort" when system
calls fail.

Thanks,
-Stolee