{"thread":{"id":"58635","subject":"[PATCH] path.c: add_to_trie - possible global-buffer-overflow fixed","startedAt":"2022-10-17T06:12:43Z","lastAt":"2022-10-17T06:12:43Z","messageCount":1,"participants":["Rubén Justo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"465067","messageId":"d28ecc16-1e73-98bb-04e7-33f4f8986986@gmail.com","threadId":"58635","inReplyTo":null,"subject":"[PATCH] path.c: add_to_trie - possible global-buffer-overflow fixed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-17T06:12:34Z","receivedAt":"2022-10-17T06:12:43Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Since 4e09cf2ac (path: optimize common dir checking, 2015-08-31) a trie\nis used to search over the common_list array.  The trie is formed from a\nwell-known list of values.  With the current implementation there can be\nproblems if the current values are rearranged or new values are\nintroduced.\n\nThis is fine:\n\n\tstatic const struct common_dir common_list[] = {\n\t\t{ 1, 1, 1, \"logs\" },\n\t\t{ 1, 0, 0, \"logs/HEAD\" },\n\t\t...\n\nBut this will lead to a global-buffer-overflow:\n\n\tstatic const struct common_dir common_list[] = {\n\t\t{ 1, 0, 0, \"logs/HEAD\" },\n\t\t{ 1, 1, 1, \"logs\" },\n\t\t...\n\nAnd this will confuse and is probably a bug:\n\n\tstatic const struct common_dir common_list[] = {\n\t\t{ 1, 1, 1, \"logs\" },\n\t\t{ 0, 0, 0, \"logs\" },\n\t\t...\n\nLet's fix the former and throw an assert on the latter.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n path.c | 19 ++++++++-----------\n 1 file changed, 8 insertions(+), 11 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex a3cfcd8a6e..a925cf073b 100644\n--- a/path.c\n+++ b/path.c\n@@ -185,13 +185,6 @@ static void *add_to_trie(struct trie *root, const char *key, void *value)\n \tvoid *old;\n \tint i;\n \n-\tif (!*key) {\n-\t\t/* we have reached the end of the key */\n-\t\told = root->value;\n-\t\troot->value = value;\n-\t\treturn old;\n-\t}\n-\n \tfor (i = 0; i < root->len; i++) {\n \t\tif (root->contents[i] == key[i])\n \t\t\tcontinue;\n@@ -209,15 +202,17 @@ static void *add_to_trie(struct trie *root, const char *key, void *value)\n \t\t\t\t\t\t   child->len);\n \t\t}\n \t\tchild->value = root->value;\n-\t\troot->value = NULL;\n+\t\troot->value = key[i]\n+\t\t\t? NULL /* Key splits the compressed section. */\n+\t\t\t: value; /* Key matches up to here. */\n \t\troot->len = i;\n \n \t\tmemset(root->children, 0, sizeof(root->children));\n \t\troot->children[(unsigned char)root->contents[i]] = child;\n \n-\t\t/* This is the newly-added child. */\n-\t\troot->children[(unsigned char)key[i]] =\n-\t\t\tmake_trie_node(key + i + 1, value);\n+\t\tif (key[i])\n+\t\t\troot->children[(unsigned char)key[i]] =\n+\t\t\t\tmake_trie_node(key + i + 1, value);\n \t\treturn NULL;\n \t}\n \n@@ -233,6 +228,8 @@ static void *add_to_trie(struct trie *root, const char *key, void *value)\n \t\t}\n \t}\n \n+\t/* We have matched the entire compressed section with the entire key */\n+\tassert(!root->value); /* Key already present in the trie? */\n \told = root->value;\n \troot->value = value;\n \treturn old;\n-- \n2.36.1\n"}]}