Re: [PATCH] notes: avoid leaking duplicate entries

From: Jeff King <peff@peff.net>
Date: Aug 25, 2019, 07:10 UTC
Message-ID: <20190825071035.GB23806@sigill.intra.peff.net>
In-Reply-To: <20190825051818.9621-1-mh@glandium.org>

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.

Show 13 quoted lines

> 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