Skip to content

Commit f269463

Browse files
committed
config_file: fix quadratic behaviour when adding config multivars
In case where we add multiple configuration entries with the same key to a diskfile backend, we always need to iterate the list of this key to find the last entry due to the list being a singly-linked list. This is obviously quadratic behaviour, and this has sure enough been found by oss-fuzz by generating a configuration file with 50k lines, where most of them have the same key. While the issue will not arise with "sane" configuration files, an adversary may trigger it by providing a crafted ".gitmodules" file, which is delivered as part of the repo and also parsed by the configuration parser. The fix is trivial: store a pointer to the last entry of the list in its head. As there are only two locations now where we append to this data structure, mainting this pointer is trivial, too. We can also optimize retrieval of a single value via `config_get`, where we previously had to chase the `next` pointer to find the last entry that was added. Using our configuration file fozzur with a corpus that has a single file with 50000 "-=" lines previously took around 21s. With this optimization the same file scans in about 0.053s, which is a nearly 400-fold improvement. But in most cases with a "normal" amount of same-named keys it's not going to matter anyway.
1 parent 695067f commit f269463

File tree

1 file changed

+17
-12
lines changed

1 file changed

+17
-12
lines changed

src/config_file.c

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525

2626
typedef struct config_entry_list {
2727
struct config_entry_list *next;
28+
struct config_entry_list *last;
2829
git_config_entry *entry;
2930
} config_entry_list;
3031

@@ -131,15 +132,11 @@ int git_config_file_normalize_section(char *start, char *end)
131132

132133
static void config_entry_list_append(config_entry_list **list, config_entry_list *entry)
133134
{
134-
config_entry_list *head = *list;
135-
136-
if (head) {
137-
while (head->next != NULL)
138-
head = head->next;
139-
head->next = entry;
140-
} else {
135+
if (*list)
136+
(*list)->last->next = entry;
137+
else
141138
*list = entry;
142-
}
139+
(*list)->last = entry;
143140
}
144141

145142
/* Add or append the new config option */
@@ -155,6 +152,17 @@ static int diskfile_entries_append(diskfile_entries *entries, git_config_entry *
155152

156153
pos = git_strmap_lookup_index(entries->map, entry->name);
157154
if (!git_strmap_valid_index(entries->map, pos)) {
155+
/*
156+
* We only ever inspect `last` from the first config
157+
* entry in a multivar. In case where this new entry is
158+
* the first one in the entry map, it will also be the
159+
* last one at the time of adding it, which is
160+
* why we set `last` here to itself. Otherwise we
161+
* do not have to set `last` and leave it set to
162+
* `NULL`.
163+
*/
164+
var->last = var;
165+
158166
git_strmap_insert(entries->map, entry->name, var, &error);
159167

160168
if (error > 0)
@@ -517,10 +525,7 @@ static int config_get(git_config_backend *cfg, const char *key, git_config_entry
517525
}
518526

519527
var = git_strmap_value_at(entry_map, pos);
520-
while (var->next)
521-
var = var->next;
522-
523-
*out = var->entry;
528+
*out = var->last->entry;
524529
(*out)->free = free_diskfile_entry;
525530
(*out)->payload = entries;
526531

0 commit comments

Comments
 (0)