| Message ID | Z0Xd-cYPNNrxwuAB@ArchLinux (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b6318cf23a21b4d1917438c642d8877fb31e7c3e |
| Headers | show |
| Series | ref-cache: fix invalid free operation in `free_ref_entry` | expand |
On Tue, Nov 26, 2024 at 10:40:57PM +0800, shejialuo wrote: > 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. > 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
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); }
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(-)