| Message ID | patch-v2-2.3-d6d04da1d9d-20211021T195133Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | refs.c + config.c: plug memory leaks | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Fix a memory leak in the error() path in handle_path_include(), this > allows us to run t1305-config-include.sh under SANITIZE=leak, > previously 4 tests there would fail. This fixes up a leak in > 9b25a0b52e0 (config: add include directive, 2012-02-06). > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > config.c | 7 +++++-- > t/t1305-config-include.sh | 1 + > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/config.c b/config.c > index 2dcbe901b6b..c5873f3a706 100644 > --- a/config.c > +++ b/config.c > @@ -148,8 +148,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc Not a problem introduced by this function, but if you look at this change with "git show -W", we'd notice that the function name on the hunk header looks strange. I think we should add a blank line before the beginning of the function. > if (!is_absolute_path(path)) { > char *slash; > > - if (!cf || !cf->path) > - return error(_("relative config includes must come from files")); > + if (!cf || !cf->path) { > + ret = error(_("relative config includes must come from files")); > + goto cleanup; > + } > > slash = find_last_dir_sep(cf->path); > if (slash) > @@ -167,6 +169,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc > ret = git_config_from_file(git_config_include, path, inc); > inc->depth--; > } > +cleanup: > strbuf_release(&buf); > free(expanded); > return ret; Quite straight-forward. At the point of the new "goto cleanup", the expanded pointer has already been established, so these two calls will release the right resource. Thanks. > diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh > index ccbb116c016..5cde79ef8c4 100755 > --- a/t/t1305-config-include.sh > +++ b/t/t1305-config-include.sh > @@ -1,6 +1,7 @@ > #!/bin/sh > > test_description='test config file include directives' > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > # Force setup_explicit_git_dir() to run until the end. This is needed
On Thu, Oct 21 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Fix a memory leak in the error() path in handle_path_include(), this >> allows us to run t1305-config-include.sh under SANITIZE=leak, >> previously 4 tests there would fail. This fixes up a leak in >> 9b25a0b52e0 (config: add include directive, 2012-02-06). >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> config.c | 7 +++++-- >> t/t1305-config-include.sh | 1 + >> 2 files changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/config.c b/config.c >> index 2dcbe901b6b..c5873f3a706 100644 >> --- a/config.c >> +++ b/config.c >> @@ -148,8 +148,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc > > Not a problem introduced by this function, but if you look at this > change with "git show -W", we'd notice that the function name on the > hunk header looks strange. I think we should add a blank line > before the beginning of the function. I think this is a bug in -W, after all if without it we we show the function context line, but with it we advance further, then that means that -W didn't find the correct function boundary.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> Not a problem introduced by this function, but if you look at this >> change with "git show -W", we'd notice that the function name on the >> hunk header looks strange. I think we should add a blank line >> before the beginning of the function. > > I think this is a bug in -W, after all if without it we we show the > function context line, but with it we advance further, then that means > that -W didn't find the correct function boundary. That's a chicken-and-egg argument, and I do not think it is a bug in "-W" nor the funcname regular expression pattern we use. We expect a blank line there and the pattern reflects that expectation, so not having an expected blank line is what causes this problem. In any case, we should add a blank linke before the beginning of the function, and of course that is obviously outside the scope of these patches.
On Fri, Oct 22 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> Not a problem introduced by this function, but if you look at this >>> change with "git show -W", we'd notice that the function name on the >>> hunk header looks strange. I think we should add a blank line >>> before the beginning of the function. >> >> I think this is a bug in -W, after all if without it we we show the >> function context line, but with it we advance further, then that means >> that -W didn't find the correct function boundary. > > That's a chicken-and-egg argument, and I do not think it is a bug in > "-W" nor the funcname regular expression pattern we use. We expect > a blank line there and the pattern reflects that expectation, so not > having an expected blank line is what causes this problem. > > In any case, we should add a blank linke before the beginning of the > function, and of course that is obviously outside the scope of these > patches. Sort of, if you were running with the patch I posted at [1] you wouldn't see the bad value at @@, but we still extend upwards with -W, which I consider a bug. I.e. both the current context we display and the over-extension there is ultimately a symptom of the same issue, which is that what we're doing with -W gets conflated with behavior that makes sense without -W, notice how if you do "git log -W" on anything that the @@ context we display is the prototype of the function /above/ the one you're likely looking at the code change in. So the blank line is the cause of the over-extension, but we'd still show the (IMO) incorrect context in either case. Anyway, as you say a discussion for some other thread. I've been meaning to get back to those patches at some point, the first problem is that our test coverage for what function context we should find when is really lacking, so any changes in that part of the xdiff code are likely to break things. I had those tests, but they got lost in some bikeshedding... 1. https://lore.kernel.org/git/20210215155020.2804-2-avarab@gmail.com/
diff --git a/config.c b/config.c index 2dcbe901b6b..c5873f3a706 100644 --- a/config.c +++ b/config.c @@ -148,8 +148,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc if (!is_absolute_path(path)) { char *slash; - if (!cf || !cf->path) - return error(_("relative config includes must come from files")); + if (!cf || !cf->path) { + ret = error(_("relative config includes must come from files")); + goto cleanup; + } slash = find_last_dir_sep(cf->path); if (slash) @@ -167,6 +169,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc ret = git_config_from_file(git_config_include, path, inc); inc->depth--; } +cleanup: strbuf_release(&buf); free(expanded); return ret; diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh index ccbb116c016..5cde79ef8c4 100755 --- a/t/t1305-config-include.sh +++ b/t/t1305-config-include.sh @@ -1,6 +1,7 @@ #!/bin/sh test_description='test config file include directives' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Force setup_explicit_git_dir() to run until the end. This is needed
Fix a memory leak in the error() path in handle_path_include(), this allows us to run t1305-config-include.sh under SANITIZE=leak, previously 4 tests there would fail. This fixes up a leak in 9b25a0b52e0 (config: add include directive, 2012-02-06). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- config.c | 7 +++++-- t/t1305-config-include.sh | 1 + 2 files changed, 6 insertions(+), 2 deletions(-)