Re: [PATCH 0/4] fix a leak with excludes_file
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 6, 2024, 17:53 UTC
- Message-ID
- <xmqq7chajs3e.fsf@gitster.g>
- In-Reply-To
- <f4af1e88-5bd9-4b3c-8691-84dbf0ca3ee2@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> We call twice to git_config(): first to get the main git-branch(1) options, > and second to get the ones related to the sequencer.
The diagnosis of the control flow that leads to overwriting the value assigned earlier to "excludes_file" is quite correct, but I find that your fix is somehow not addressing the real issue.
We used to assume that excludes_file *always* points at a piece of memory that somebody else owns. But seeing the two places that assign to the variable, both actually give the variable a piece of memory allocated for the caller.
- The one in config.c that reads core.excludesfile via git_config_pathname(), which calls interpolate_path(), whose return value is always allocated.
- The one in dir.c that sets up standard excludes uses xdg_config_home() that returns a value obtained from mkpathdup().
The core of the fix ought to be just like this, but to lose the cast that discards the constness away, the fallouts will be large (if you remove "const" from the first parameter to git_config_pathname(), the compiler will tell you where the fallouts are).
We may want to eventually fix the type of the first parameter of the git_config_pathmame() function, but I somehow doubt that your approach gets us any closer to that future. Adding a parallel API next to the existing git_config_pathname() and interpolate_path() and convert only these callers that touch excludes_file would not help other callers that hold the pointer git_config_pathname() returned.
config.c | 4 +++- t/t7300-clean.sh | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git c/config.c w/config.c index eebce8c7e0..ae3652b08f 100644 --- c/config.c +++ w/config.c @@ -1584,8 +1584,10 @@ static int git_default_core_config(const char *var, const char *value, if (!strcmp(var, "core.askpass")) return git_config_string(&askpass_program, var, value); - if (!strcmp(var, "core.excludesfile")) + if (!strcmp(var, "core.excludesfile")) { + free((char *)excludes_file); return git_config_pathname(&excludes_file, var, value); + } if (!strcmp(var, "core.whitespace")) { if (!value) diff --git c/t/t7300-clean.sh w/t/t7300-clean.sh index 1f7201eb60..0aae0dee67 100755 --- c/t/t7300-clean.sh +++ w/t/t7300-clean.sh @@ -5,6 +5,7 @@ test_description='git clean basic tests' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh git config clean.requireForce no