| Message ID | 20240407010704.GK868358@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git_config_string() considered harmful | expand |
On Sat, Apr 6, 2024 at 9:07 PM Jeff King <peff@peff.net> wrote: > We feed items into ignore_revs_file_list in two ways: from config and > from the command-line. Items from the command-line point to argv entries > that we don't own, but items from config are hea-allocated strings whose > ownership is handed to the string list when we insert them. s/hea/heap/ > Because the string_list is declared with "NODUP", our string_list_clear() > call will not free the entries themselves. This is the right thing for > the argv entries, but means that we leak the allocated config entries. > > Let's declare the string list as owning its own strings, which means > that the argv entries will be copied. > > For the config entries we would ideally use string_list_append_nodup() > to just hand off ownership of our strings. But we insert them into the > sorted list with string_list_insert(), which doesn't have a nodup > variant. Since this isn't a hot path, we can accept that the path > interpolation will produce a temporary string which is then freed. > > Signed-off-by: Jeff King <peff@peff.net>
diff --git a/builtin/blame.c b/builtin/blame.c index 0b07ceb110..fa7f8fce09 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -67,7 +67,7 @@ static int no_whole_file_rename; static int show_progress; static char repeated_meta_color[COLOR_MAXLEN]; static int coloring_mode; -static struct string_list ignore_revs_file_list = STRING_LIST_INIT_NODUP; +static struct string_list ignore_revs_file_list = STRING_LIST_INIT_DUP; static int mark_unblamable_lines; static int mark_ignored_lines; @@ -725,6 +725,7 @@ static int git_blame_config(const char *var, const char *value, if (ret) return ret; string_list_insert(&ignore_revs_file_list, str); + free(str); return 0; } if (!strcmp(var, "blame.markunblamablelines")) {
We feed items into ignore_revs_file_list in two ways: from config and from the command-line. Items from the command-line point to argv entries that we don't own, but items from config are hea-allocated strings whose ownership is handed to the string list when we insert them. Because the string_list is declared with "NODUP", our string_list_clear() call will not free the entries themselves. This is the right thing for the argv entries, but means that we leak the allocated config entries. Let's declare the string list as owning its own strings, which means that the argv entries will be copied. For the config entries we would ideally use string_list_append_nodup() to just hand off ownership of our strings. But we insert them into the sorted list with string_list_insert(), which doesn't have a nodup variant. Since this isn't a hot path, we can accept that the path interpolation will produce a temporary string which is then freed. Signed-off-by: Jeff King <peff@peff.net> --- Not strictly related to this series, but something I noticed while converting this spot in an earlier patch. I had thought originally we could switch to avoid the extra copy altogether: if (!value) return config_error_nonbool(); string_list_insert(&ignore_revs_file_list, value); but of course that is missing the call to interpolate_path(). I imagine that it could also be further refactored to append while reading the config, and then sort after. That's more efficient overall, and would let us use append_nodup(). builtin/blame.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)