From: Elijah Newren <newren@gmail.com>
Date: Tue, 25 Aug 2026 22:14:37 GMT
Subject: Re: [PATCH v2 3/7] trace2: remove use of xstrdup()
Message-ID: <CABPp-BH1TeDTeqddZw+cvzou+3PRgw+HNpYF2JnhMTSBp9qfbQ@mail.gmail.com>
In-Reply-To: <ec447a6a778a5c49344346df54b434a96c792082.1787684181.git.gitgitgadget@gmail.com>

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)

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.