{"thread":{"id":"51727","subject":"[PATCH] notes: avoid leaking duplicate entries","startedAt":"2019-08-25T05:20:07Z","lastAt":"2019-08-26T01:56:56Z","messageCount":4,"participants":["Mike Hommey","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"381085","messageId":"20190825051818.9621-1-mh@glandium.org","threadId":"51727","inReplyTo":null,"subject":"[PATCH] notes: avoid leaking duplicate entries","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-08-25T05:18:18Z","receivedAt":"2019-08-25T05:20:07Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"When add_note is called multiple times with the same key/value pair, the\nleaf_node it creates is leaked by notes_tree_insert.\n---\n notes.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/notes.c b/notes.c\nindex 75c028b300..ec35f5b551 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -269,8 +269,10 @@ static int note_tree_insert(struct notes_tree *t, struct int_node *tree,\n \t\tcase PTR_TYPE_NOTE:\n \t\t\tif (oideq(&l->key_oid, &entry->key_oid)) {\n \t\t\t\t/* skip concatenation if l == entry */\n-\t\t\t\tif (oideq(&l->val_oid, &entry->val_oid))\n+\t\t\t\tif (oideq(&l->val_oid, &entry->val_oid)) {\n+\t\t\t\t\tfree(entry);\n \t\t\t\t\treturn 0;\n+\t\t\t\t}\n \n \t\t\t\tret = combine_notes(&l->val_oid,\n \t\t\t\t\t\t &entry->val_oid);\n-- \n2.23.0\n\n"},{"id":"381087","messageId":"20190825071035.GB23806@sigill.intra.peff.net","threadId":"51727","inReplyTo":"20190825051818.9621-1-mh@glandium.org","subject":"Re: [PATCH] notes: avoid leaking duplicate entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-25T07:10:35Z","receivedAt":"2019-08-25T07:10:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 25, 2019 at 02:18:18PM +0900, Mike Hommey wrote:\n\n> When add_note is called multiple times with the same key/value pair, the\n> leaf_node it creates is leaked by notes_tree_insert.\n\nMakes sense.\n\n> diff --git a/notes.c b/notes.c\n> index 75c028b300..ec35f5b551 100644\n> --- a/notes.c\n> +++ b/notes.c\n> @@ -269,8 +269,10 @@ static int note_tree_insert(struct notes_tree *t, struct int_node *tree,\n> \t\tcase PTR_TYPE_NOTE:\n> \t\t\tif (oideq(&l->key_oid, &entry->key_oid)) {\n> \t\t\t\t/* skip concatenation if l == entry */\n> -\t\t\t\tif (oideq(&l->val_oid, &entry->val_oid))\n> +\t\t\t\tif (oideq(&l->val_oid, &entry->val_oid)) {\n> +\t\t\t\t\tfree(entry);\n> \t\t\t\t\treturn 0;\n> +\t\t\t\t}\n\n\"entry\" is passed in by the caller. Does anybody try to insert, and then\nafter the insertion continue to access the entry?\n\nThe only case I could find is this one in load_subtree():\n\n if (note_tree_insert(t, node, n, l, type,\n combine_notes_concatenate))\n die(\"Failed to load %s %s into notes tree \"\n \"from %s\",\n type == PTR_TYPE_NOTE ? \"note\" : \"subtree\",\n oid_to_hex(&l->key_oid), t->ref);\n\nIf we fail to insert, we'll try to access the key_oid of the entry we\npassed in, which might have been freed. But your patch is OK, because it\nonly touches a code path where we always return success.\n\nCuriously, I think the existing case a few lines below your patch is\nwrong, though:\n\n ret = combine_notes(&l->val_oid,\n &entry->val_oid);\n if (!ret && is_null_oid(&l->val_oid))\n note_tree_remove(t, tree, n, entry);\n free(entry);\n return ret;\n\nIf combining the notes fails, we'll free the entry and return an error\ncode, and then load_subtree() will access the freed memory. I think we\ncould just object_oid instead.\n\n-Peff\n"},{"id":"381088","messageId":"20190825071951.GC23806@sigill.intra.peff.net","threadId":"51727","inReplyTo":"20190825071035.GB23806@sigill.intra.peff.net","subject":"[PATCH] notes: avoid potential use-after-free during insertion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-25T07:19:51Z","receivedAt":"2019-08-25T07:19:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 25, 2019 at 03:10:35AM -0400, Jeff King wrote:\n\n> Curiously, I think the existing case a few lines below your patch is\n> wrong, though:\n> \n> ret = combine_notes(&l->val_oid,\n> &entry->val_oid);\n> if (!ret && is_null_oid(&l->val_oid))\n> note_tree_remove(t, tree, n, entry);\n> free(entry);\n> return ret;\n> \n> If combining the notes fails, we'll free the entry and return an error\n> code, and then load_subtree() will access the freed memory. I think we\n> could just object_oid instead.\n\nHere's a patch. It's textually independent of yours, but I think it\nmakes sense to apply the two on a single branch.\n\nNo test, as I didn't bother to figure out what it takes to trigger the\nerror message, and the solution is (hopefully) an obvious improvement\neven if it can't be triggered in practice.\n\n-- >8 --\nSubject: [PATCH] notes: avoid potential use-after-free during insertion\n\nThe note_tree_insert() function may free the leaf_node struct we pass in\n(e.g., if it's a duplicate, or if it needs to be combined with an\nexisting note).\n\nMost callers are happy with this, as they assume that ownership of the\nstruct is handed off. But in load_subtree(), if we see an error we'll\nuse the handed-off struct's key_oid to generate the die() message,\npotentially accessing freed memory.\n\nWe can easily fix this by instead using the original oid that we copied\ninto the leaf_node struct.\n\nSigned-off-by: Jeff King \n---\n notes.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/notes.c b/notes.c\nindex 75c028b300..3477e8e554 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -453,17 +453,17 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,\n \t\tl = xcalloc(1, sizeof(*l));\n \t\toidcpy(&l->key_oid, &object_oid);\n \t\toidcpy(&l->val_oid, &entry.oid);\n \t\tif (note_tree_insert(t, node, n, l, type,\n \t\t\t\t combine_notes_concatenate))\n \t\t\tdie(\"Failed to load %s %s into notes tree \"\n \t\t\t \"from %s\",\n \t\t\t type == PTR_TYPE_NOTE ? \"note\" : \"subtree\",\n-\t\t\t oid_to_hex(&l->key_oid), t->ref);\n+\t\t\t oid_to_hex(&object_oid), t->ref);\n \n \t\tcontinue;\n \n handle_non_note:\n \t\t/*\n \t\t * Determine full path for this non-note entry. The\n \t\t * filename is already found in entry.path, but the\n \t\t * directory part of the path must be deduced from the\n-- \n2.23.0.478.g23872bed7d\n\n"},{"id":"381151","messageId":"20190826015502.qbgn557xjylju4o6@glandium.org","threadId":"51727","inReplyTo":"20190825051818.9621-1-mh@glandium.org","subject":"Re: [PATCH] notes: avoid leaking duplicate entries","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-08-26T01:55:02Z","receivedAt":"2019-08-26T01:56:56Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sun, Aug 25, 2019 at 02:18:18PM +0900, Mike Hommey wrote:\n> When add_note is called multiple times with the same key/value pair, the\n> leaf_node it creates is leaked by notes_tree_insert.\n\nFor completeness, since I realized it was missing:\nSigned-off-by: Mike Hommey \n\nMike\n"}]}