Skip to content

Commit 4bb216a

Browse files
committed
config.c: do sanity checks before xmemdupz()
Adjust the code added in 1ff21c0 (config: store "git -c" variables using more robust format, 2021-01-12) to avoid allocating a "key" until after we've done the sanity checks on it. There reason for allocating it this in the function is because the "env_name" pointer was about to be incremented, let's instead note our position at that point. The key length is the number of characters before the first "=". As a result of this early allocation we'd have a memory leak as reported by valgrind, just not the "still reachable" kind we usually care about[1] with SANITIZE=leak: $ valgrind --leak-check=full --show-leak-kinds=all ./git --config-env=foo.flag= config --bool foo.flag [...] ==6540== 13 bytes in 1 blocks are still reachable in loss record 3 of 17 ==6540== at 0x48437B4: malloc (vg_replace_malloc.c:381) ==6540== by 0x40278B: do_xmalloc (wrapper.c:51) ==6540== by 0x402884: do_xmallocz (wrapper.c:85) ==6540== by 0x4028C0: xmallocz (wrapper.c:93) ==6540== by 0x4028FD: xmemdupz (wrapper.c:109) ==6540== by 0x402962: xstrndup (wrapper.c:115) ==6540== by 0x342F53: strip_path_suffix (path.c:1300) ==6540== by 0x2B4C89: system_prefix (exec-cmd.c:50) ==6540== by 0x2B5057: system_path (exec-cmd.c:268) ==6540== by 0x2C5E17: git_setup_gettext (gettext.c:109) ==6540== by 0x21DC57: main (common-main.c:44) Since the "key" was reachable when the "die()" ran the semantics of SANITIZE=leak wouldn't show it, but "clang" at higher optimization levels would optimize this to the point of thinking the variable wasn't reachable, e.g. with Debian clang 14.0.6-2 with CFLAGS="-O3 -g": $ ./git --config-env=foo.flag= config --bool foo.flag fatal: missing environment variable name for configuration 'foo.flag' ================================================================= ==18018==ERROR: LeakSanitizer: detected memory leaks Direct leak of 9 byte(s) in 1 object(s) allocated from: #0 0x55e40aad7cd2 in __interceptor_malloc (/home/avar/g/git/git+0x78cd2) (BuildId: b89fa8f797ccb02ef1148beed300071a5f9b9ab1) #1 0x55e40ad41071 in do_xmalloc /home/avar/g/git/wrapper.c:51:8 #2 0x55e40ad41071 in do_xmallocz /home/avar/g/git/wrapper.c:85:8 #3 0x55e40ad41071 in xmallocz /home/avar/g/git/wrapper.c:93:9 #4 0x55e40ad41071 in xmemdupz /home/avar/g/git/wrapper.c:109:16 #5 0x55e40abec960 in git_config_push_env /home/avar/g/git/config.c:521:8 git#6 0x55e40aadb8b9 in handle_options /home/avar/g/git/git.c:268:4 #7 0x55e40aada68d in cmd_main /home/avar/g/git/git.c:896:2 git#8 0x55e40abae219 in main /home/avar/g/git/common-main.c:57:11 git#9 0x7fbb5287e209 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 git#10 0x7fbb5287e2bb in __libc_start_main csu/../csu/libc-start.c:389:3 git#11 0x55e40aaac130 in _start (/home/avar/g/git/git+0x4d130) (BuildId: b89fa8f797ccb02ef1148beed300071a5f9b9ab1) SUMMARY: LeakSanitizer: 9 byte(s) leaked in 1 allocation(s). Aborted Whatever clang has to say about it it makes sense to save ourselves the xmemdupz() if we can help it, but it's worth noting why this came up. The actual meaningful fix here is that we don't need to do this allocation at all. The only reason it's needed is because there hasn't been a variant of "sq_quote_buf()" in quote.c that takes a "len" parameter. In previous commits we added one, and will move to using it in the subsequent commit, but let's first make this smaller change. 1. https://lore.kernel.org/git/[email protected]/ Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
1 parent eb53389 commit 4bb216a

File tree

1 file changed

+3
-1
lines changed

1 file changed

+3
-1
lines changed

config.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -571,13 +571,14 @@ void git_config_push_parameter(const char *text)
571571
void git_config_push_env(const char *spec)
572572
{
573573
char *key;
574+
size_t len;
574575
const char *env_name;
575576
const char *env_value;
576577

577578
env_name = strrchr(spec, '=');
578579
if (!env_name)
579580
die(_("invalid config format: %s"), spec);
580-
key = xmemdupz(spec, env_name - spec);
581+
len = env_name - spec;
581582
env_name++;
582583
if (!*env_name)
583584
die(_("missing environment variable name for configuration '%.*s'"),
@@ -588,6 +589,7 @@ void git_config_push_env(const char *spec)
588589
die(_("missing environment variable '%s' for configuration '%.*s'"),
589590
env_name, (int)(env_name - spec - 1), spec);
590591

592+
key = xmemdupz(spec, len);
591593
git_config_push_split_parameter(key, env_value);
592594
free(key);
593595
}

0 commit comments

Comments
 (0)