| Message ID | 5d09959b6426e53a68e1bce547f9507bdf21bcde.1724159575.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.5) | expand |
Patrick Steinhardt <ps@pks.im> writes: > We read a bunch of configs in `use_sideband_colors()` to configure the > colors that Git should use. We never free the strings read from the > config though, causing memory leaks. Fix those. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > sideband.c | 8 +++++--- > t/t5409-colorize-remote-messages.sh | 1 + > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/sideband.c b/sideband.c > index 5d8907151fe..deb6ec0a8b7 100644 > --- a/sideband.c > +++ b/sideband.c > @@ -30,7 +30,7 @@ static int use_sideband_colors(void) > > const char *key = "color.remote"; > struct strbuf sb = STRBUF_INIT; > - char *value; > + char *value = NULL; Hmph, this is a bit unfortunate. If there is no configuration variable, on the first call to this function, we come to the end of if/else cascade and we need to free. Another possibility to convey the intention better may be to assign NULL to value after the "we already know the value, so return early" took place. > @@ -43,15 +43,17 @@ static int use_sideband_colors(void) > } else { > use_sideband_colors_cached = GIT_COLOR_AUTO; > } > + FREE_AND_NULL(value); This can be a simple "free(value)"; I do not see the need to clear the variable at this point. > for (i = 0; i < ARRAY_SIZE(keywords); i++) { > strbuf_reset(&sb); > strbuf_addf(&sb, "%s.%s", key, keywords[i].keyword); > if (git_config_get_string(sb.buf, &value)) > continue; > - if (color_parse(value, keywords[i].color)) > - continue; > + color_parse(value, keywords[i].color); > + FREE_AND_NULL(value); Likewise. I do not see the need to clear. We only come here after we got something from gti_config_get_string(). That value may or may not fail color_parse(), but when we reach this point, value always has something we need to free, not some leftover value from the previous iteration. The patch is _not_ wrong per-se, though. Thanks.
On Tue, Aug 20, 2024 at 04:52:59PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > We read a bunch of configs in `use_sideband_colors()` to configure the > > colors that Git should use. We never free the strings read from the > > config though, causing memory leaks. Fix those. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > sideband.c | 8 +++++--- > > t/t5409-colorize-remote-messages.sh | 1 + > > 2 files changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/sideband.c b/sideband.c > > index 5d8907151fe..deb6ec0a8b7 100644 > > --- a/sideband.c > > +++ b/sideband.c > > @@ -30,7 +30,7 @@ static int use_sideband_colors(void) > > > > const char *key = "color.remote"; > > struct strbuf sb = STRBUF_INIT; > > - char *value; > > + char *value = NULL; > > Hmph, this is a bit unfortunate. If there is no configuration > variable, on the first call to this function, we come to the end of > if/else cascade and we need to free. > > Another possibility to convey the intention better may be to assign > NULL to value after the "we already know the value, so return early" > took place. There is an even better option: just use `git_config_get_string_tmp()`. Then we don't have to worry about freeing the strings at all. Patrick
diff --git a/sideband.c b/sideband.c index 5d8907151fe..deb6ec0a8b7 100644 --- a/sideband.c +++ b/sideband.c @@ -30,7 +30,7 @@ static int use_sideband_colors(void) const char *key = "color.remote"; struct strbuf sb = STRBUF_INIT; - char *value; + char *value = NULL; int i; if (use_sideband_colors_cached >= 0) @@ -43,15 +43,17 @@ static int use_sideband_colors(void) } else { use_sideband_colors_cached = GIT_COLOR_AUTO; } + FREE_AND_NULL(value); for (i = 0; i < ARRAY_SIZE(keywords); i++) { strbuf_reset(&sb); strbuf_addf(&sb, "%s.%s", key, keywords[i].keyword); if (git_config_get_string(sb.buf, &value)) continue; - if (color_parse(value, keywords[i].color)) - continue; + color_parse(value, keywords[i].color); + FREE_AND_NULL(value); } + strbuf_release(&sb); return use_sideband_colors_cached; } diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh index fa5de4500a4..516b22fd963 100755 --- a/t/t5409-colorize-remote-messages.sh +++ b/t/t5409-colorize-remote-messages.sh @@ -2,6 +2,7 @@ test_description='remote messages are colorized on the client' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
We read a bunch of configs in `use_sideband_colors()` to configure the colors that Git should use. We never free the strings read from the config though, causing memory leaks. Fix those. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- sideband.c | 8 +++++--- t/t5409-colorize-remote-messages.sh | 1 + 2 files changed, 6 insertions(+), 3 deletions(-)