ref-cache: fix invalid free operation in `free_ref_entry`
PATCH 1/1 ref-cache: fix invalid free operation in `free_ref_entry`
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.
--- refs/ref-cache.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
refs/ref-cache.c +2 -1
@@ -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); }
Signature · 1 line
2.47.0
Quoted 14 lines · shejialuo
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.
Quoted 8 lines
The anonymous union in the "
ref_entry" has two members: one is "structref_value", another is "structref_dir". On a 64-bit machine, the size of "structref_dir" is 32 bytes, which is smaller than the 48-byte size of "structref_value". And the offset of "referent" field in "structref_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.
Quoted 14 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 <[email protected]>--- refs/ref-cache.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)refs/ref-cache.c +0
-0
refs/ref-cache.c static void free_ref_entry(struct ref_entry *entry) · line 75
Quoted 9 lines
refs/ref-cache.c +2
-1@@ -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