{"thread":{"id":"62548","subject":"[PATCH] ref-cache: fix invalid free operation in `free_ref_entry`","startedAt":"2024-11-26T14:40:43Z","lastAt":"2024-11-27T05:50:27Z","messageCount":2,"participants":["shejialuo","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"508184","messageId":"Z0Xd-cYPNNrxwuAB@ArchLinux","threadId":"62548","inReplyTo":null,"subject":"[PATCH] ref-cache: fix invalid free operation in `free_ref_entry`","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-11-26T14:40:57Z","receivedAt":"2024-11-26T14:40:43Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"In cfd971520e (refs: keep track of unresolved reference value in\niterators, 2024-08-09), we added a new field \"referent\" into the \"struct\nref\" structure. In order to free the \"referent\", we unconditionally\nfreed the \"referent\" by simply adding a \"free\" statement.\n\nHowever, this is a bad usage. Because when ref entry is either directory\nor loose ref, we will always execute the following statement:\n\n  free(entry->u.value.referent);\n\nThis does not make sense. We should never access the \"entry->u.value\"\nfield when \"entry\" is a directory. However, the change obviously doesn't\nbreak the tests. Let's analysis why.\n\nThe anonymous union in the \"ref_entry\" has two members: one is \"struct\nref_value\", another is \"struct ref_dir\". On a 64-bit machine, the size\nof \"struct ref_dir\" is 32 bytes, which is smaller than the 48-byte size\nof \"struct ref_value\". And the offset of \"referent\" field in \"struct\nref_value\" is 40 bytes. So, whenever we create a new \"ref_entry\" for a\ndirectory, we will leave the offset from 40 bytes to 48 bytes untouched,\nwhich means the value for this memory is zero (NULL). It's OK to free a\nNULL pointer, but this is merely a coincidence of memory layout.\n\nTo fix this issue, we now ensure that \"free(entry->u.value.referent)\" is\nonly called when \"entry->flag\" indicates that it represents a loose\nreference and not a directory to avoid the invalid memory operation.\n\nSigned-off-by: shejialuo <shejialuo@gmail.com>\n---\n refs/ref-cache.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex 35bae7e05d..02f09e4df8 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -68,8 +68,9 @@ static void free_ref_entry(struct ref_entry *entry)\n \t\t * trigger the reading of loose refs.\n \t\t */\n \t\tclear_ref_dir(&entry->u.subdir);\n+\t} else {\n+\t\tfree(entry->u.value.referent);\n \t}\n-\tfree(entry->u.value.referent);\n \tfree(entry);\n }\n \n-- \n2.47.0\n\n"},{"id":"508212","messageId":"Z0azEzigqr6DlitP@pks.im","threadId":"62548","inReplyTo":"Z0Xd-cYPNNrxwuAB@ArchLinux","subject":"Re: [PATCH] ref-cache: fix invalid free operation in `free_ref_entry`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-27T05:50:11Z","receivedAt":"2024-11-27T05:50:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Nov 26, 2024 at 10:40:57PM +0800, shejialuo wrote:\n> In cfd971520e (refs: keep track of unresolved reference value in\n> iterators, 2024-08-09), we added a new field \"referent\" into the \"struct\n> ref\" structure. In order to free the \"referent\", we unconditionally\n> freed the \"referent\" by simply adding a \"free\" statement.\n> \n> However, this is a bad usage. Because when ref entry is either directory\n> or loose ref, we will always execute the following statement:\n> \n>   free(entry->u.value.referent);\n> \n> This does not make sense. We should never access the \"entry->u.value\"\n> field when \"entry\" is a directory. However, the change obviously doesn't\n> break the tests. Let's analysis why.\n> \n> The anonymous union in the \"ref_entry\" has two members: one is \"struct\n> ref_value\", another is \"struct ref_dir\". On a 64-bit machine, the size\n> of \"struct ref_dir\" is 32 bytes, which is smaller than the 48-byte size\n> of \"struct ref_value\". And the offset of \"referent\" field in \"struct\n> ref_value\" is 40 bytes. So, whenever we create a new \"ref_entry\" for a\n> directory, we will leave the offset from 40 bytes to 48 bytes untouched,\n> which means the value for this memory is zero (NULL). It's OK to free a\n> NULL pointer, but this is merely a coincidence of memory layout.\n\nMakes sense.\n\n> To fix this issue, we now ensure that \"free(entry->u.value.referent)\" is\n> only called when \"entry->flag\" indicates that it represents a loose\n> reference and not a directory to avoid the invalid memory operation.\n> \n> Signed-off-by: shejialuo <shejialuo@gmail.com>\n> ---\n>  refs/ref-cache.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/refs/ref-cache.c b/refs/ref-cache.c\n> index 35bae7e05d..02f09e4df8 100644\n> --- a/refs/ref-cache.c\n> +++ b/refs/ref-cache.c\n> @@ -68,8 +68,9 @@ static void free_ref_entry(struct ref_entry *entry)\n>  \t\t * trigger the reading of loose refs.\n>  \t\t */\n>  \t\tclear_ref_dir(&entry->u.subdir);\n> +\t} else {\n> +\t\tfree(entry->u.value.referent);\n>  \t}\n> -\tfree(entry->u.value.referent);\n>  \tfree(entry);\n>  }\n\nAnd the fix looks obviously good to me.\n\nThanks for catching this!\n\nPatrick\n"}]}