| Message ID | a34c90a5527cb45ec89a0ad44dbca1d61705a0ea.1722933642.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.4) | expand |
On Tue Aug 6, 2024 at 7:00 PM AEST, Patrick Steinhardt wrote: > Refactor the code so that we initialize the value with another array. > This allows us to free the value in case the string is not pointing to > that constant array anymore. > > diff --git a/environment.c b/environment.c > index 5cea2c9f54..8297c6e37b 100644 > --- a/environment.c > +++ b/environment.c > @@ -113,7 +113,8 @@ int protect_ntfs = PROTECT_NTFS_DEFAULT; > * The character that begins a commented line in user-editable file > * that is subject to stripspace. > */ > -const char *comment_line_str = "#"; > +const char comment_line_str_default[] = "#"; > +const char *comment_line_str = comment_line_str_default; > int auto_comment_line_char; > > /* Parallel index stat data preload? */ Is my understanding correct that `comment_line_str` is now just a pointer to the `comment_line_str_default` array, and thus can be freed once we're done with it?
On Wed, Aug 07, 2024 at 05:11:28PM +1000, James Liu wrote: > On Tue Aug 6, 2024 at 7:00 PM AEST, Patrick Steinhardt wrote: > > Refactor the code so that we initialize the value with another array. > > This allows us to free the value in case the string is not pointing to > > that constant array anymore. > > > > diff --git a/environment.c b/environment.c > > index 5cea2c9f54..8297c6e37b 100644 > > --- a/environment.c > > +++ b/environment.c > > @@ -113,7 +113,8 @@ int protect_ntfs = PROTECT_NTFS_DEFAULT; > > * The character that begins a commented line in user-editable file > > * that is subject to stripspace. > > */ > > -const char *comment_line_str = "#"; > > +const char comment_line_str_default[] = "#"; > > +const char *comment_line_str = comment_line_str_default; > > int auto_comment_line_char; > > > > /* Parallel index stat data preload? */ > > Is my understanding correct that `comment_line_str` is now just a > pointer to the `comment_line_str_default` array, and thus can be freed > once we're done with it? Not quite. By default, `comment_line_str` also points to comment_line_str_default`, which is a string constant and thus neither of these variables can be free'd. But what this split allows us to do is to check whether `comment_line_str` has changed from the default, and thus we can conditionall free it when it does not point to the default value anymore. Now that I revisit this commit I'm not quite happy with it anymore. We still need to have the cast, which is somewhat awkward. I think the better solution is to instead have a `comment_line_str_allocated` variable that is non-constant. I'll adapt the code accordingly. An even better solution would be to have `struct strbuf` provide an initializer that populates it with a string constant. But that feels like a larger undertaking, so I'll leave that for the future. Patrick
Patrick Steinhardt <ps@pks.im> writes: > Now that I revisit this commit I'm not quite happy with it anymore. We > still need to have the cast, which is somewhat awkward. I think the > better solution is to instead have a `comment_line_str_allocated` > variable that is non-constant. I'll adapt the code accordingly. > > An even better solution would be to have `struct strbuf` provide an > initializer that populates it with a string constant. But that feels > like a larger undertaking, so I'll leave that for the future. FWIW, I found the "now we have a variable to refer to the address of the string constant, we can compare to detect if we allocated and need to free" in this round is a good place to stop. I view the approach to use an auxiliary variable *_allocated is a regression compared to what we see here. The approach makes it easy to forget to futz it when an allocated piece of memory is assigned to the main variable.
diff --git a/config.c b/config.c index 6421894614..63e0211c7d 100644 --- a/config.c +++ b/config.c @@ -1596,6 +1596,8 @@ static int git_default_core_config(const char *var, const char *value, else if (value[0]) { if (strchr(value, '\n')) return error(_("%s cannot contain newline"), var); + if (comment_line_str != comment_line_str_default) + free((char *) comment_line_str); comment_line_str = xstrdup(value); auto_comment_line_char = 0; } else diff --git a/environment.c b/environment.c index 5cea2c9f54..8297c6e37b 100644 --- a/environment.c +++ b/environment.c @@ -113,7 +113,8 @@ int protect_ntfs = PROTECT_NTFS_DEFAULT; * The character that begins a commented line in user-editable file * that is subject to stripspace. */ -const char *comment_line_str = "#"; +const char comment_line_str_default[] = "#"; +const char *comment_line_str = comment_line_str_default; int auto_comment_line_char; /* Parallel index stat data preload? */ diff --git a/environment.h b/environment.h index e9f01d4d11..5e5d9a8045 100644 --- a/environment.h +++ b/environment.h @@ -8,6 +8,7 @@ struct strvec; * The character that begins a commented line in user-editable file * that is subject to stripspace. */ +extern const char comment_line_str_default[]; extern const char *comment_line_str; extern int auto_comment_line_char;
When the comment line character has been specified multiple times in the configuration, then `git_default_core_config()` will cause a memory leak because it unconditionally copies the string into `comment_line_str` without free'ing the previous value. In fact, it can't easily free the value in the first place because it may contain a string constant. Refactor the code so that we initialize the value with another array. This allows us to free the value in case the string is not pointing to that constant array anymore. This memory leak is being hit in t3404. As there are still other memory leaks in that file we cannot yet mark it as passing with leak checking enabled. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- config.c | 2 ++ environment.c | 3 ++- environment.h | 1 + 3 files changed, 5 insertions(+), 1 deletion(-)