Re: [PATCH v2 3/7] trace2: remove use of xstrdup()
From: Elijah Newren <hidden>
Date: 2026-08-25 22:14:50
On Tue, Aug 25, 2026 at 11:58 AM Derrick Stolee via GitGitGadget [off-list ref] 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.
[...]
quoted hunk ↗ jump to hunk
--- 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.