notes: avoid leaking duplicate entries

Commit Message

Mike Hommey Aug. 25, 2019, 5:18 a.m. UTC

When add_note is called multiple times with the same key/value pair, the
leaf_node it creates is leaked by notes_tree_insert.
---
 notes.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Jeff King Aug. 25, 2019, 7:10 a.m. UTC | #1

On Sun, Aug 25, 2019 at 02:18:18PM +0900, Mike Hommey wrote:

> When add_note is called multiple times with the same key/value pair, the
> leaf_node it creates is leaked by notes_tree_insert.

Makes sense.

> diff --git a/notes.c b/notes.c
> index 75c028b300..ec35f5b551 100644
> --- a/notes.c
> +++ b/notes.c
> @@ -269,8 +269,10 @@ static int note_tree_insert(struct notes_tree *t, struct int_node *tree,
>  		case PTR_TYPE_NOTE:
>  			if (oideq(&l->key_oid, &entry->key_oid)) {
>  				/* skip concatenation if l == entry */
> -				if (oideq(&l->val_oid, &entry->val_oid))
> +				if (oideq(&l->val_oid, &entry->val_oid)) {
> +					free(entry);
>  					return 0;
> +				}

"entry" is passed in by the caller. Does anybody try to insert, and then
after the insertion continue to access the entry?

The only case I could find is this one in load_subtree():

        if (note_tree_insert(t, node, n, l, type,
                             combine_notes_concatenate))
                die("Failed to load %s %s into notes tree "
                    "from %s",
                    type == PTR_TYPE_NOTE ? "note" : "subtree",
                    oid_to_hex(&l->key_oid), t->ref);

If we fail to insert, we'll try to access the key_oid of the entry we
passed in, which might have been freed. But your patch is OK, because it
only touches a code path where we always return success.

Curiously, I think the existing case a few lines below your patch is
wrong, though:

        ret = combine_notes(&l->val_oid,
                            &entry->val_oid);
        if (!ret && is_null_oid(&l->val_oid))
                note_tree_remove(t, tree, n, entry);
        free(entry);
        return ret;

If combining the notes fails, we'll free the entry and return an error
code, and then load_subtree() will access the freed memory. I think we
could just object_oid instead.

-Peff

Mike Hommey Aug. 26, 2019, 1:55 a.m. UTC | #2

On Sun, Aug 25, 2019 at 02:18:18PM +0900, Mike Hommey wrote:
> When add_note is called multiple times with the same key/value pair, the
> leaf_node it creates is leaked by notes_tree_insert.

For completeness, since I realized it was missing:
Signed-off-by: Mike Hommey <[e-mail]>

Mike

Patch

diff --git a/notes.c b/notes.c
index 75c028b300..ec35f5b551 100644
--- a/notes.c
+++ b/notes.c
@@ -269,8 +269,10 @@  static int note_tree_insert(struct notes_tree *t, struct int_node *tree,
 		case PTR_TYPE_NOTE:
 			if (oideq(&l->key_oid, &entry->key_oid)) {
 				/* skip concatenation if l == entry */
-				if (oideq(&l->val_oid, &entry->val_oid))
+				if (oideq(&l->val_oid, &entry->val_oid)) {
+					free(entry);
 					return 0;
+				}
 
 				ret = combine_notes(&l->val_oid,
 						    &entry->val_oid);