git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2] merge: fix cache_entry use-after-free

From
Junio C Hamano <gitster@pobox.com>
Date
Oct 12, 2015, 22:28 UTC
Message-ID
<xmqqio6bbu2w.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<1444687413-928-1-git-send-email-dturner@twitter.com>
David Turner <dturner@twopensource.com> writes:
Show 14 quoted lines
> From: Keith McGuigan <kmcguigan@twitter.com>
>
> During merges, we would previously free entries that we no longer need
> in the destination index.  But those entries might also be stored in
> the dir_entry cache, and when a later call to add_to_index found them,
> they would be used after being freed.
>
> To prevent this, add a ref count for struct cache_entry.  Whenever
> a cache entry is added to a data structure, the ref count is incremented;
> when it is removed from the data structure, it is decremented.  When
> it hits zero, the cache_entry is freed.
>
> Signed-off-by: Keith McGuigan <kmcguigan@twitter.com>
> ---
I'll forge your "messenger's sign-off" here ;-)
Show 14 quoted lines
> diff --git a/unpack-trees.c b/unpack-trees.c
> index f932e80..1a0a637 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -606,8 +606,10 @@ static int unpack_nondirectories(int n, unsigned long mask,
>  					o);
>  		for (i = 0; i < n; i++) {
>  			struct cache_entry *ce = src[i + o->merge];
> -			if (ce != o->df_conflict_entry)
> -				free(ce);
> +			if (ce != o->df_conflict_entry) {
> +				drop_ce_ref(ce);
> +				src[i + o->merge] = NULL;
> +			}

This one smelled iffy. I think it is safe because the caller does not look at src[] other than src[0] after this function returns, and this setting to NULL happens only when o->merge is set to 1, so I do not think this is buggy, but at the same time I do not think setting to NULL is necessary.

Other than that, looks nice.  Thanks.
Previous: David TurnerNext: David Turner
Message 2 of 6 in “merge: fix cache_entry use-after-free”
  1. merge: fix cache_entry use-after-freeDavid Turner, Oct 12, 2015
  2. Junio C HamanoOct 12, 2015
  3. David TurnerOct 13, 2015
  4. Junio C HamanoOct 13, 2015
  5. Junio C HamanoOct 14, 2015
  6. David TurnerOct 14, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.