threads / patch / 62548

patchref-cache: fix invalid free operation in `free_ref_entry`

Subject: [PATCH] ref-cache: fix invalid free operation in `free_ref_entry`

## tl;dr

2 messages between Nov 26, 2024 and Nov 27, 2024. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

shejialuo· Nov 26, 2024, 14:40 UTC · lore

In cfd971520e (refs: keep track of unresolved reference value in iterators, 2024-08-09), we added a new field "referent" into the "struct ref" structure. In order to free the "referent", we unconditionally freed the "referent" by simply adding a "free" statement.

However, this is a bad usage. Because when ref entry is either directory or loose ref, we will always execute the following statement:

  free(entry->u.value.referent);

This does not make sense. We should never access the "entry->u.value" field when "entry" is a directory. However, the change obviously doesn't break the tests. Let's analysis why.

The anonymous union in the "ref_entry" has two members: one is "struct ref_value", another is "struct ref_dir". On a 64-bit machine, the size of "struct ref_dir" is 32 bytes, which is smaller than the 48-byte size of "struct ref_value". And the offset of "referent" field in "struct ref_value" is 40 bytes. So, whenever we create a new "ref_entry" for a directory, we will leave the offset from 40 bytes to 48 bytes untouched, which means the value for this memory is zero (NULL). It's OK to free a NULL pointer, but this is merely a coincidence of memory layout.

To fix this issue, we now ensure that "free(entry->u.value.referent)" is only called when "entry->flag" indicates that it represents a loose reference and not a directory to avoid the invalid memory operation.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 refs/ref-cache.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to refs/ref-cache.c +2 −1
diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index 35bae7e05d..02f09e4df8 100644
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -68,8 +68,9 @@ static void free_ref_entry(struct ref_entry *entry)
 		 * trigger the reading of loose refs.
 		 */
 		clear_ref_dir(&entry->u.subdir);
+	} else {
+		free(entry->u.value.referent);
 	}
-	free(entry->u.value.referent);
 	free(entry);
 }
 
-- 
2.47.0
Patrick Steinhardt· Nov 27, 2024, 05:50 UTC · re: shejialuo · lore

Re: [PATCH] ref-cache: fix invalid free operation in `free_ref_entry`

On Tue, Nov 26, 2024 at 10:40:57PM +0800, shejialuo wrote:
Show 22 quoted lines
> In cfd971520e (refs: keep track of unresolved reference value in
> iterators, 2024-08-09), we added a new field "referent" into the "struct
> ref" structure. In order to free the "referent", we unconditionally
> freed the "referent" by simply adding a "free" statement.
> 
> However, this is a bad usage. Because when ref entry is either directory
> or loose ref, we will always execute the following statement:
> 
>   free(entry->u.value.referent);
> 
> This does not make sense. We should never access the "entry->u.value"
> field when "entry" is a directory. However, the change obviously doesn't
> break the tests. Let's analysis why.
> 
> The anonymous union in the "ref_entry" has two members: one is "struct
> ref_value", another is "struct ref_dir". On a 64-bit machine, the size
> of "struct ref_dir" is 32 bytes, which is smaller than the 48-byte size
> of "struct ref_value". And the offset of "referent" field in "struct
> ref_value" is 40 bytes. So, whenever we create a new "ref_entry" for a
> directory, we will leave the offset from 40 bytes to 48 bytes untouched,
> which means the value for this memory is zero (NULL). It's OK to free a
> NULL pointer, but this is merely a coincidence of memory layout.
Makes sense.
Show 23 quoted lines
> To fix this issue, we now ensure that "free(entry->u.value.referent)" is
> only called when "entry->flag" indicates that it represents a loose
> reference and not a directory to avoid the invalid memory operation.
> 
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
>  refs/ref-cache.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
> index 35bae7e05d..02f09e4df8 100644
> --- a/refs/ref-cache.c
> +++ b/refs/ref-cache.c
> @@ -68,8 +68,9 @@ static void free_ref_entry(struct ref_entry *entry)
>  		 * trigger the reading of loose refs.
>  		 */
>  		clear_ref_dir(&entry->u.subdir);
> +	} else {
> +		free(entry->u.value.referent);
>  	}
> -	free(entry->u.value.referent);
>  	free(entry);
>  }
And the fix looks obviously good to me.
Thanks for catching this!
Patrick

← back to recent threads