{"thread":{"id":"64041","subject":"[PATCH] reset slab_alloc and state fields in clear_alloc_state()","startedAt":"2025-08-26T19:57:52Z","lastAt":"2025-09-05T19:37:32Z","messageCount":28,"participants":["ノウラ | Flare via GitGitGadget","Junio C Hamano","Torsten Bögershausen","Jeff King","ノウラ | Flare"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"524996","messageId":"pull.2040.git.git.1756238268790.gitgitgadget@gmail.com","threadId":"64041","inReplyTo":null,"subject":"[PATCH] reset slab_alloc and state fields in clear_alloc_state()","fromName":"ノウラ | Flare via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-26T19:57:48Z","receivedAt":"2025-08-26T19:57:52Z","isPatch":true,"sender":{"key":"nouraellm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26191219?v=4"},"body":"From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9?= \n\nclear_alloc_state() freed all slabs and nulled the slabs pointer but\nleft slab_alloc, nr, and p unchanged. If the alloc_state is reused,\nALLOC_GROW() can wrongly assume that the slab array is already\nallocated because slab_alloc still holds a stale nonzero capacity.\nIn that case s->slabs remains NULL and the next dereference writes\nthrough a NULL pointer, causing undefined behavior.\n\nTo fix this, we reset slab_alloc, nr, and p to zero/NULL after\nfreeing the slabs. This leaves alloc_state in a consistent empty\nstate for reuse and avoids dangling pointers.\n\nSigned-off-by: Noura EL ALLAM \n---\n Reset slab_alloc and state fields in clear_alloc_state()\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v1\nPull-Request: https://github.com/git/git/pull/2040\n\n alloc.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/alloc.c b/alloc.c\nindex 377e80f5dda..6bf9421c123 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -49,6 +49,9 @@ void clear_alloc_state(struct alloc_state *s)\n \t}\n \n \tFREE_AND_NULL(s->slabs);\n+\ts->slab_alloc = 0;\n+\ts->nr = 0;\n+\ts->p = NULL;\n }\n \n static inline void *alloc_node(struct alloc_state *s, size_t node_size)\n\nbase-commit: f814da676ae46aac5be0a98b99373a76dee6cedb\n-- \ngitgitgadget\n"},{"id":"525021","messageId":"xmqq8qj5frfs.fsf@gitster.g","threadId":"64041","inReplyTo":"pull.2040.git.git.1756238268790.gitgitgadget@gmail.com","subject":"Re: [PATCH] reset slab_alloc and state fields in clear_alloc_state()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-27T02:07:19Z","receivedAt":"2025-08-27T02:07:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ノウラ | Flare via GitGitGadget\" writes:\n\n> From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9?= \n\nThis name used on the in-body From: line must match who signs off\nthe patch below on Signed-off-by: line.\n\n> clear_alloc_state() freed all slabs and nulled the slabs pointer but\n> left slab_alloc, nr, and p unchanged. If the alloc_state is reused,\n> ALLOC_GROW() can wrongly assume that the slab array is already\n> allocated because slab_alloc still holds a stale nonzero capacity.\n> In that case s->slabs remains NULL and the next dereference writes\n> through a NULL pointer, causing undefined behavior.\n\nLet's not give such a misuse-prone API to consumers, then. How\nabout doing something like this instead?\n\nAs Documentation/CodingGuidelines says the API functions for\nsubsystem S should in general be called S_, let's rename\nallocate_alloc_state() to alloc_state_alloc(), get rid of the \"just\nclear\" function and make it alloc_state_free_and_null() to do just\nthat.\n\n alloc.c | 7 +++++--\n alloc.h | 4 ++--\n object.c | 26 +++++++++++---------------\n 3 files changed, 18 insertions(+), 19 deletions(-)\n\ndiff --git c/alloc.c w/alloc.c\nindex 377e80f5dd..3a5d0b2bd8 100644\n--- c/alloc.c\n+++ w/alloc.c\n@@ -36,19 +36,22 @@ struct alloc_state {\n \tint slab_nr, slab_alloc;\n };\n \n-struct alloc_state *allocate_alloc_state(void)\n+struct alloc_state *alloc_state_alloc(void)\n {\n \treturn xcalloc(1, sizeof(struct alloc_state));\n }\n \n-void clear_alloc_state(struct alloc_state *s)\n+void alloc_state_free_and_null(struct alloc_state **s_)\n {\n+\tstruct alloc_state *s = *s_;\n+\n \twhile (s->slab_nr > 0) {\n \t\ts->slab_nr--;\n \t\tfree(s->slabs[s->slab_nr]);\n \t}\n \n \tFREE_AND_NULL(s->slabs);\n+\tFREE_AND_NULL(*s_);\n }\n \n static inline void *alloc_node(struct alloc_state *s, size_t node_size)\ndiff --git c/alloc.h w/alloc.h\nindex 3f4a0ad310..cd6ed16ffb 100644\n--- c/alloc.h\n+++ w/alloc.h\n@@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);\n void *alloc_tag_node(struct repository *r);\n void *alloc_object_node(struct repository *r);\n \n-struct alloc_state *allocate_alloc_state(void);\n-void clear_alloc_state(struct alloc_state *s);\n+struct alloc_state *alloc_state_alloc(void);\n+void alloc_state_free_and_null(struct alloc_state **s);\n \n #endif\ndiff --git c/object.c w/object.c\nindex c1553ee433..4469755ea6 100644\n--- c/object.c\n+++ w/object.c\n@@ -517,11 +517,11 @@ struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)\n \tmemset(o, 0, sizeof(*o));\n \n \to->repo = repo;\n-\to->blob_state = allocate_alloc_state();\n-\to->tree_state = allocate_alloc_state();\n-\to->commit_state = allocate_alloc_state();\n-\to->tag_state = allocate_alloc_state();\n-\to->object_state = allocate_alloc_state();\n+\to->blob_state = alloc_state_alloc();\n+\to->tree_state = alloc_state_alloc();\n+\to->commit_state = alloc_state_alloc();\n+\to->tag_state = alloc_state_alloc();\n+\to->object_state = alloc_state_alloc();\n \n \to->is_shallow = -1;\n \tCALLOC_ARRAY(o->shallow_stat, 1);\n@@ -573,16 +573,12 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)\n \to->buffer_slab = NULL;\n \n \tparsed_object_pool_reset_commit_grafts(o);\n-\tclear_alloc_state(o->blob_state);\n-\tclear_alloc_state(o->tree_state);\n-\tclear_alloc_state(o->commit_state);\n-\tclear_alloc_state(o->tag_state);\n-\tclear_alloc_state(o->object_state);\n+\talloc_state_free_and_null(&o->blob_state);\n+\talloc_state_free_and_null(&o->tree_state);\n+\talloc_state_free_and_null(&o->commit_state);\n+\talloc_state_free_and_null(&o->tag_state);\n+\talloc_state_free_and_null(&o->object_state);\n+\n \tstat_validity_clear(o->shallow_stat);\n-\tFREE_AND_NULL(o->blob_state);\n-\tFREE_AND_NULL(o->tree_state);\n-\tFREE_AND_NULL(o->commit_state);\n-\tFREE_AND_NULL(o->tag_state);\n-\tFREE_AND_NULL(o->object_state);\n \tFREE_AND_NULL(o->shallow_stat);\n }\n"},{"id":"525084","messageId":"pull.2040.v2.git.git.1756337312918.gitgitgadget@gmail.com","threadId":"64041","inReplyTo":"pull.2040.git.git.1756238268790.gitgitgadget@gmail.com","subject":"[PATCH v2] alloc: fix dangling pointer in alloc_state cleanup","fromName":"ノウラ | Flare via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-27T23:28:32Z","receivedAt":"2025-08-27T23:28:36Z","isPatch":true,"sender":{"key":"nouraellm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26191219?v=4"},"body":"From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=\n \n\nclear_alloc_state() freed all slabs and nulled the slabs pointer but\nleft slab_alloc, nr, and p unchanged. If the alloc_state is reused,\nALLOC_GROW() can wrongly assume that the slab array is already\nallocated because slab_alloc still holds a stale nonzero capacity.\nIn that case s->slabs remains NULL and the next dereference writes\nthrough a NULL pointer, causing undefined behavior.\n\nTo fix this, this patch:\n\n- Renames allocate_alloc_state() → alloc_state_alloc().\n- Replaces the “just clear” API with\n alloc_state_free_and_null(struct alloc_state **s_),\n which frees all slabs and the alloc_state itself,\n and then nulls the caller’s pointer.\n- Updates call sites to use the new helpers and drops\n redundant FREE_AND_NULL() calls.\n\nThis makes the alloc_state lifecycle API harder to misuse,\neliminates stale-capacity state,\nand aligns naming with project conventions.\n\nSigned-off-by: ノウラ | Flare \n---\n alloc: fix dangling pointer in alloc_state cleanup\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v2\nPull-Request: https://github.com/git/git/pull/2040\n\nRange-diff vs v1:\n\n 1: 643f14e6225 < -: ----------- reset slab_alloc and state fields in clear_alloc_state()\n -: ----------- > 1: 0b980850bdf alloc: fix dangling pointer in alloc_state cleanup\n\n\n alloc.c | 9 +++++++--\n alloc.h | 4 ++--\n object.c | 26 ++++++++++----------------\n 3 files changed, 19 insertions(+), 20 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex 377e80f5dda..c1065551c07 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -36,19 +36,24 @@ struct alloc_state {\n \tint slab_nr, slab_alloc;\n };\n \n-struct alloc_state *allocate_alloc_state(void)\n+struct alloc_state *alloc_state_alloc(void)\n {\n \treturn xcalloc(1, sizeof(struct alloc_state));\n }\n \n-void clear_alloc_state(struct alloc_state *s)\n+void alloc_state_free_and_null(struct alloc_state **s_)\n {\n+\tstruct alloc_state *s = *s_;\n+\n+\tif (!s_ || !*s_) return;\n+\n \twhile (s->slab_nr > 0) {\n \t\ts->slab_nr--;\n \t\tfree(s->slabs[s->slab_nr]);\n \t}\n \n \tFREE_AND_NULL(s->slabs);\n+\tFREE_AND_NULL(*s_);\n }\n \n static inline void *alloc_node(struct alloc_state *s, size_t node_size)\ndiff --git a/alloc.h b/alloc.h\nindex 3f4a0ad310a..87a47a97095 100644\n--- a/alloc.h\n+++ b/alloc.h\n@@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);\n void *alloc_tag_node(struct repository *r);\n void *alloc_object_node(struct repository *r);\n \n-struct alloc_state *allocate_alloc_state(void);\n-void clear_alloc_state(struct alloc_state *s);\n+struct alloc_state *alloc_state_alloc(void);\n+void alloc_state_free_and_null(struct alloc_state **s_);\n \n #endif\ndiff --git a/object.c b/object.c\nindex c1553ee4330..986114a6dba 100644\n--- a/object.c\n+++ b/object.c\n@@ -517,12 +517,11 @@ struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)\n \tmemset(o, 0, sizeof(*o));\n \n \to->repo = repo;\n-\to->blob_state = allocate_alloc_state();\n-\to->tree_state = allocate_alloc_state();\n-\to->commit_state = allocate_alloc_state();\n-\to->tag_state = allocate_alloc_state();\n-\to->object_state = allocate_alloc_state();\n-\n+\to->blob_state = alloc_state_alloc();\n+\to->tree_state = alloc_state_alloc();\n+\to->commit_state = alloc_state_alloc();\n+\to->tag_state = alloc_state_alloc();\n+\to->object_state = alloc_state_alloc();\n \to->is_shallow = -1;\n \tCALLOC_ARRAY(o->shallow_stat, 1);\n \n@@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)\n \to->buffer_slab = NULL;\n \n \tparsed_object_pool_reset_commit_grafts(o);\n-\tclear_alloc_state(o->blob_state);\n-\tclear_alloc_state(o->tree_state);\n-\tclear_alloc_state(o->commit_state);\n-\tclear_alloc_state(o->tag_state);\n-\tclear_alloc_state(o->object_state);\n+\talloc_state_free_and_null(&o->blob_state);\n+\talloc_state_free_and_null(&o->tree_state);\n+\talloc_state_free_and_null(&o->commit_state);\n+\talloc_state_free_and_null(&o->tag_state);\n+\talloc_state_free_and_null(&o->object_state);\n \tstat_validity_clear(o->shallow_stat);\n-\tFREE_AND_NULL(o->blob_state);\n-\tFREE_AND_NULL(o->tree_state);\n-\tFREE_AND_NULL(o->commit_state);\n-\tFREE_AND_NULL(o->tag_state);\n-\tFREE_AND_NULL(o->object_state);\n \tFREE_AND_NULL(o->shallow_stat);\n }\n\nbase-commit: f814da676ae46aac5be0a98b99373a76dee6cedb\n-- \ngitgitgadget\n"},{"id":"525140","messageId":"20250828192949.GA18641@tb-raspi4","threadId":"64041","inReplyTo":"pull.2040.v2.git.git.1756337312918.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] alloc: fix dangling pointer in alloc_state cleanup","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2025-08-28T19:29:49Z","receivedAt":"2025-08-28T19:29:57Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Wed, Aug 27, 2025 at 11:28:32PM +0000, ノウラ | Flare via GitGitGadget wrote:\n> From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=\n> \n> \n> clear_alloc_state() freed all slabs and nulled the slabs pointer but\n> left slab_alloc, nr, and p unchanged. If the alloc_state is reused,\n> ALLOC_GROW() can wrongly assume that the slab array is already\n> allocated because slab_alloc still holds a stale nonzero capacity.\n> In that case s->slabs remains NULL and the next dereference writes\n> through a NULL pointer, causing undefined behavior.\nThis is good.\n\n> \n> To fix this, this patch:\nStyle nit, we tend to use the \"imperative form\" here in Git,\nlike this:\n\n- Rename allocate_alloc_state() → alloc_state_alloc().\n- Replace ...\n- Update ...\n> \n> - Renames allocate_alloc_state() → alloc_state_alloc().\n> - Replaces the “just clear” API with\n> alloc_state_free_and_null(struct alloc_state **s_),\n> which frees all slabs and the alloc_state itself,\n> and then nulls the caller’s pointer.\n> - Updates call sites to use the new helpers and drops\n> redundant FREE_AND_NULL() calls.\n> \n> This makes the alloc_state lifecycle API harder to misuse,\n> eliminates stale-capacity state,\n> and aligns naming with project conventions.\n"},{"id":"525142","messageId":"xmqqtt1rut1w.fsf@gitster.g","threadId":"64041","inReplyTo":"20250828192949.GA18641@tb-raspi4","subject":"Re: [PATCH v2] alloc: fix dangling pointer in alloc_state cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-28T19:47:55Z","receivedAt":"2025-08-28T19:47:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> On Wed, Aug 27, 2025 at 11:28:32PM +0000, ノウラ | Flare via GitGitGadget wrote:\n>> From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=\n>> \n>> \n>> clear_alloc_state() freed all slabs and nulled the slabs pointer but\n>> left slab_alloc, nr, and p unchanged. If the alloc_state is reused,\n>> ALLOC_GROW() can wrongly assume that the slab array is already\n>> allocated because slab_alloc still holds a stale nonzero capacity.\n>> In that case s->slabs remains NULL and the next dereference writes\n>> through a NULL pointer, causing undefined behavior.\n> This is good.\n>\n>> \n>> To fix this, this patch:\n> Style nit, we tend to use the \"imperative form\" here in Git,\n> like this:\n>\n> - Rename allocate_alloc_state() → alloc_state_alloc().\n> - Replace ...\n> - Update ...\n\nThanks. We also tend to avoid bulleted list.\n"},{"id":"525144","messageId":"xmqqo6rzusex.fsf@gitster.g","threadId":"64041","inReplyTo":"xmqqtt1rut1w.fsf@gitster.g","subject":"Re: [PATCH v2] alloc: fix dangling pointer in alloc_state cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-28T20:01:42Z","receivedAt":"2025-08-28T20:01:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> Style nit, we tend to use the \"imperative form\" here in Git,\n>> like this:\n>>\n>> - Rename allocate_alloc_state() → alloc_state_alloc().\n>> - Replace ...\n>> - Update ...\n>\n> Thanks. We also tend to avoid bulleted list.\n\nPerhaps it needs a bit of clarification. \n\nIf you truly require a bulleted list to enumerate what you did in\nthe patch, then you may be doing too many things in a single patch.\n\nBut in this case, the changes do not even have to be broken down\ninto bullets. If you change a called function, it goes without\nsaying that you have to adjust existing callers. Perhaps something\nlike\n\n All callers of clear_alloc_state() immediately free what they\n cleared, so currently it does not hurt anybody that the\n alloc_state is left in an unreusable state, but it is an\n error-prone API. Replace it with a new function that clears but\n in addition frees the structure, as well as NULLing the pointer\n that points at it and adjust existing callers.\n\n While at it, rename allocate_alloc_state() and name the new\n function alloc_state_free_and_null(), to follow more closely the\n function naming convention specified in the CodingGuidelines\n (namely, functions about S are named with S_ prefix and then\n verb).\n\nshould be more in line with what we usually do.\n\nThanks.\n"},{"id":"525198","messageId":"pull.2040.v3.git.git.1756472406487.gitgitgadget@gmail.com","threadId":"64041","inReplyTo":"pull.2040.v2.git.git.1756337312918.gitgitgadget@gmail.com","subject":"[PATCH v3] alloc: fix dangling pointer in alloc_state cleanup","fromName":"ノウラ | Flare via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-29T13:00:06Z","receivedAt":"2025-08-29T13:00:10Z","isPatch":true,"sender":{"key":"nouraellm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26191219?v=4"},"body":"From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=\n \n\nAll callers of clear_alloc_state() immediately free what they\ncleared, so currently it does not hurt anybody that the\nalloc_state is left in an unreusable state, but it is an\nerror-prone API. Replace it with a new function that clears but\nin addition frees the structure, as well as NULLing the pointer\nthat points at it and adjust existing callers.\n\nWhile at it, rename allocate_alloc_state() and name the new\nfunction alloc_state_free_and_null(), to follow more closely the\nfunction naming convention specified in the CodingGuidelines\n(namely, functions about S are named with S_ prefix and then\nverb).\n\nSigned-off-by: ノウラ | Flare \n---\n alloc: fix dangling pointer in alloc_state cleanup\n \n cc: Torsten Bögershausen tboegi@web.de\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v3\nPull-Request: https://github.com/git/git/pull/2040\n\nRange-diff vs v2:\n\n 1: 0b980850bd ! 1: c521b44adb alloc: fix dangling pointer in alloc_state cleanup\n @@ Metadata\n ## Commit message ##\n alloc: fix dangling pointer in alloc_state cleanup\n \n - clear_alloc_state() freed all slabs and nulled the slabs pointer but\n - left slab_alloc, nr, and p unchanged. If the alloc_state is reused,\n - ALLOC_GROW() can wrongly assume that the slab array is already\n - allocated because slab_alloc still holds a stale nonzero capacity.\n - In that case s->slabs remains NULL and the next dereference writes\n - through a NULL pointer, causing undefined behavior.\n + All callers of clear_alloc_state() immediately free what they\n + cleared, so currently it does not hurt anybody that the\n + alloc_state is left in an unreusable state, but it is an\n + error-prone API. Replace it with a new function that clears but\n + in addition frees the structure, as well as NULLing the pointer\n + that points at it and adjust existing callers.\n \n - To fix this, this patch:\n -\n - - Renames allocate_alloc_state() → alloc_state_alloc().\n - - Replaces the “just clear” API with\n - alloc_state_free_and_null(struct alloc_state **s_),\n - which frees all slabs and the alloc_state itself,\n - and then nulls the caller’s pointer.\n - - Updates call sites to use the new helpers and drops\n - redundant FREE_AND_NULL() calls.\n -\n - This makes the alloc_state lifecycle API harder to misuse,\n - eliminates stale-capacity state,\n - and aligns naming with project conventions.\n + While at it, rename allocate_alloc_state() and name the new\n + function alloc_state_free_and_null(), to follow more closely the\n + function naming convention specified in the CodingGuidelines\n + (namely, functions about S are named with S_ prefix and then\n + verb).\n \n Signed-off-by: ノウラ | Flare \n \n\n\n alloc.c | 9 +++++++--\n alloc.h | 4 ++--\n object.c | 26 ++++++++++----------------\n 3 files changed, 19 insertions(+), 20 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex 377e80f5dd..c1065551c0 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -36,19 +36,24 @@ struct alloc_state {\n \tint slab_nr, slab_alloc;\n };\n \n-struct alloc_state *allocate_alloc_state(void)\n+struct alloc_state *alloc_state_alloc(void)\n {\n \treturn xcalloc(1, sizeof(struct alloc_state));\n }\n \n-void clear_alloc_state(struct alloc_state *s)\n+void alloc_state_free_and_null(struct alloc_state **s_)\n {\n+\tstruct alloc_state *s = *s_;\n+\n+\tif (!s_ || !*s_) return;\n+\n \twhile (s->slab_nr > 0) {\n \t\ts->slab_nr--;\n \t\tfree(s->slabs[s->slab_nr]);\n \t}\n \n \tFREE_AND_NULL(s->slabs);\n+\tFREE_AND_NULL(*s_);\n }\n \n static inline void *alloc_node(struct alloc_state *s, size_t node_size)\ndiff --git a/alloc.h b/alloc.h\nindex 3f4a0ad310..87a47a9709 100644\n--- a/alloc.h\n+++ b/alloc.h\n@@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);\n void *alloc_tag_node(struct repository *r);\n void *alloc_object_node(struct repository *r);\n \n-struct alloc_state *allocate_alloc_state(void);\n-void clear_alloc_state(struct alloc_state *s);\n+struct alloc_state *alloc_state_alloc(void);\n+void alloc_state_free_and_null(struct alloc_state **s_);\n \n #endif\ndiff --git a/object.c b/object.c\nindex c1553ee433..986114a6db 100644\n--- a/object.c\n+++ b/object.c\n@@ -517,12 +517,11 @@ struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)\n \tmemset(o, 0, sizeof(*o));\n \n \to->repo = repo;\n-\to->blob_state = allocate_alloc_state();\n-\to->tree_state = allocate_alloc_state();\n-\to->commit_state = allocate_alloc_state();\n-\to->tag_state = allocate_alloc_state();\n-\to->object_state = allocate_alloc_state();\n-\n+\to->blob_state = alloc_state_alloc();\n+\to->tree_state = alloc_state_alloc();\n+\to->commit_state = alloc_state_alloc();\n+\to->tag_state = alloc_state_alloc();\n+\to->object_state = alloc_state_alloc();\n \to->is_shallow = -1;\n \tCALLOC_ARRAY(o->shallow_stat, 1);\n \n@@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)\n \to->buffer_slab = NULL;\n \n \tparsed_object_pool_reset_commit_grafts(o);\n-\tclear_alloc_state(o->blob_state);\n-\tclear_alloc_state(o->tree_state);\n-\tclear_alloc_state(o->commit_state);\n-\tclear_alloc_state(o->tag_state);\n-\tclear_alloc_state(o->object_state);\n+\talloc_state_free_and_null(&o->blob_state);\n+\talloc_state_free_and_null(&o->tree_state);\n+\talloc_state_free_and_null(&o->commit_state);\n+\talloc_state_free_and_null(&o->tag_state);\n+\talloc_state_free_and_null(&o->object_state);\n \tstat_validity_clear(o->shallow_stat);\n-\tFREE_AND_NULL(o->blob_state);\n-\tFREE_AND_NULL(o->tree_state);\n-\tFREE_AND_NULL(o->commit_state);\n-\tFREE_AND_NULL(o->tag_state);\n-\tFREE_AND_NULL(o->object_state);\n \tFREE_AND_NULL(o->shallow_stat);\n }\n\nbase-commit: f814da676ae46aac5be0a98b99373a76dee6cedb\n-- \ngitgitgadget\n"},{"id":"525418","messageId":"20250903111829.GA1877518@coredump.intra.peff.net","threadId":"64041","inReplyTo":"pull.2040.v3.git.git.1756472406487.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] alloc: fix dangling pointer in alloc_state cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-03T11:18:29Z","receivedAt":"2025-09-03T11:18:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 29, 2025 at 01:00:06PM +0000, ノウラ | Flare via GitGitGadget wrote:\n\n> +void alloc_state_free_and_null(struct alloc_state **s_)\n> {\n> +\tstruct alloc_state *s = *s_;\n> +\n> +\tif (!s_ || !*s_) return;\n> +\n\nCoverity complains that there's a NULL check here for \"s_\", but we'll\nhave already dereferenced it in the initializer for \"s\".\n\nI don't think any caller passes NULL, so you can't trigger a segfault in\npractice. But the code is kind of misleading. Should it just be:\n\n if (!*s_)\n\treturn;\n\n? Or even just \"if (!s)\".\n\n-Peff\n"},{"id":"525469","messageId":"xmqqtt1jci4k.fsf@gitster.g","threadId":"64041","inReplyTo":"20250903111829.GA1877518@coredump.intra.peff.net","subject":"Re: [PATCH v3] alloc: fix dangling pointer in alloc_state cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-03T21:59:23Z","receivedAt":"2025-09-03T21:59:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Aug 29, 2025 at 01:00:06PM +0000, ノウラ | Flare via GitGitGadget wrote:\n>\n>> +void alloc_state_free_and_null(struct alloc_state **s_)\n>> {\n>> +\tstruct alloc_state *s = *s_;\n>> +\n>> +\tif (!s_ || !*s_) return;\n>> +\n>\n> Coverity complains that there's a NULL check here for \"s_\", but we'll\n> have already dereferenced it in the initializer for \"s\".\n>\n> I don't think any caller passes NULL, so you can't trigger a segfault in\n> practice. But the code is kind of misleading. Should it just be:\n>\n> if (!*s_)\n> \treturn;\n>\n> ? Or even just \"if (!s)\".\n\nYup, I like that.\n\nThe primary point of s_ (parameter with a trailing underscore) is\nthat we would want to use it as-is as little as possible. When we\ntalk about the pointer to alloc_state in this function (not the\nlocation such a pointer is stored at), we should use \"s\" (not\n\"*s_\").\n\nThanks for sanity checking. This may have been my breakage.\n\n\n"},{"id":"525473","messageId":"pull.2040.v4.git.git.1756941427825.gitgitgadget@gmail.com","threadId":"64041","inReplyTo":"pull.2040.v3.git.git.1756472406487.gitgitgadget@gmail.com","subject":"[PATCH v4] alloc: fix dangling pointer in alloc_state cleanup","fromName":"ノウラ | Flare via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-09-03T23:17:07Z","receivedAt":"2025-09-03T23:17:10Z","isPatch":true,"sender":{"key":"nouraellm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26191219?v=4"},"body":"From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=\n \n\nAll callers of clear_alloc_state() immediately free what they\ncleared, so currently it does not hurt anybody that the\nalloc_state is left in an unreusable state, but it is an\nerror-prone API. Replace it with a new function that clears but\nin addition frees the structure, as well as NULLing the pointer\nthat points at it and adjust existing callers.\n\nWhile at it, rename allocate_alloc_state() and name the new\nfunction alloc_state_free_and_null(), to follow more closely the\nfunction naming convention specified in the CodingGuidelines\n(namely, functions about S are named with S_ prefix and then\nverb).\n\nSigned-off-by: ノウラ | Flare \n---\n alloc: fix dangling pointer in alloc_state cleanup\n \n cc: Torsten Bögershausen tboegi@web.de cc: Jeff King peff@peff.net\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v4\nPull-Request: https://github.com/git/git/pull/2040\n\nRange-diff vs v3:\n\n 1: c521b44adb ! 1: 82d228661d alloc: fix dangling pointer in alloc_state cleanup\n @@ alloc.c: struct alloc_state {\n -void clear_alloc_state(struct alloc_state *s)\n +void alloc_state_free_and_null(struct alloc_state **s_)\n {\n -+\tstruct alloc_state *s = *s_;\n ++\tstruct alloc_state *s;\n +\n +\tif (!s_ || !*s_) return;\n ++\n ++\ts = *s_;\n +\n \twhile (s->slab_nr > 0) {\n \t\ts->slab_nr--;\n\n\n alloc.c | 11 +++++++++--\n alloc.h | 4 ++--\n object.c | 26 ++++++++++----------------\n 3 files changed, 21 insertions(+), 20 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex 377e80f5dd..1483697ca8 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -36,19 +36,26 @@ struct alloc_state {\n \tint slab_nr, slab_alloc;\n };\n \n-struct alloc_state *allocate_alloc_state(void)\n+struct alloc_state *alloc_state_alloc(void)\n {\n \treturn xcalloc(1, sizeof(struct alloc_state));\n }\n \n-void clear_alloc_state(struct alloc_state *s)\n+void alloc_state_free_and_null(struct alloc_state **s_)\n {\n+\tstruct alloc_state *s;\n+\n+\tif (!s_ || !*s_) return;\n+\n+\ts = *s_;\n+\n \twhile (s->slab_nr > 0) {\n \t\ts->slab_nr--;\n \t\tfree(s->slabs[s->slab_nr]);\n \t}\n \n \tFREE_AND_NULL(s->slabs);\n+\tFREE_AND_NULL(*s_);\n }\n \n static inline void *alloc_node(struct alloc_state *s, size_t node_size)\ndiff --git a/alloc.h b/alloc.h\nindex 3f4a0ad310..87a47a9709 100644\n--- a/alloc.h\n+++ b/alloc.h\n@@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);\n void *alloc_tag_node(struct repository *r);\n void *alloc_object_node(struct repository *r);\n \n-struct alloc_state *allocate_alloc_state(void);\n-void clear_alloc_state(struct alloc_state *s);\n+struct alloc_state *alloc_state_alloc(void);\n+void alloc_state_free_and_null(struct alloc_state **s_);\n \n #endif\ndiff --git a/object.c b/object.c\nindex c1553ee433..986114a6db 100644\n--- a/object.c\n+++ b/object.c\n@@ -517,12 +517,11 @@ struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)\n \tmemset(o, 0, sizeof(*o));\n \n \to->repo = repo;\n-\to->blob_state = allocate_alloc_state();\n-\to->tree_state = allocate_alloc_state();\n-\to->commit_state = allocate_alloc_state();\n-\to->tag_state = allocate_alloc_state();\n-\to->object_state = allocate_alloc_state();\n-\n+\to->blob_state = alloc_state_alloc();\n+\to->tree_state = alloc_state_alloc();\n+\to->commit_state = alloc_state_alloc();\n+\to->tag_state = alloc_state_alloc();\n+\to->object_state = alloc_state_alloc();\n \to->is_shallow = -1;\n \tCALLOC_ARRAY(o->shallow_stat, 1);\n \n@@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)\n \to->buffer_slab = NULL;\n \n \tparsed_object_pool_reset_commit_grafts(o);\n-\tclear_alloc_state(o->blob_state);\n-\tclear_alloc_state(o->tree_state);\n-\tclear_alloc_state(o->commit_state);\n-\tclear_alloc_state(o->tag_state);\n-\tclear_alloc_state(o->object_state);\n+\talloc_state_free_and_null(&o->blob_state);\n+\talloc_state_free_and_null(&o->tree_state);\n+\talloc_state_free_and_null(&o->commit_state);\n+\talloc_state_free_and_null(&o->tag_state);\n+\talloc_state_free_and_null(&o->object_state);\n \tstat_validity_clear(o->shallow_stat);\n-\tFREE_AND_NULL(o->blob_state);\n-\tFREE_AND_NULL(o->tree_state);\n-\tFREE_AND_NULL(o->commit_state);\n-\tFREE_AND_NULL(o->tag_state);\n-\tFREE_AND_NULL(o->object_state);\n \tFREE_AND_NULL(o->shallow_stat);\n }\n\nbase-commit: f814da676ae46aac5be0a98b99373a76dee6cedb\n-- \ngitgitgadget\n"},{"id":"525485","messageId":"xmqqh5xibqvu.fsf@gitster.g","threadId":"64041","inReplyTo":"pull.2040.v4.git.git.1756941427825.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] alloc: fix dangling pointer in alloc_state cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-04T07:47:49Z","receivedAt":"2025-09-04T07:47:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ノウラ | Flare via GitGitGadget\" writes:\n\n> +void alloc_state_free_and_null(struct alloc_state **s_)\n> {\n> +\tstruct alloc_state *s;\n> +\n> +\tif (!s_ || !*s_) return;\n\nI still do not see the point of this check. If the caller passes a\nNULL pointer, when they are expected to pass the address of a\npointer variable so that the struct the pointer points at is cleared\nand freed, and the pointer variable is NULLed, it is called a\nprogrammer error and they deserve a segfault. Why would it be\nbetter to sweep such an error under the rug by returning without\nanything? It would delay discovery of such a bug, but for what\ngain?\n\n"},{"id":"525511","messageId":"c8137063-c928-45a1-aae5-9de2b35e5150@gmail.com","threadId":"64041","inReplyTo":"xmqqh5xibqvu.fsf@gitster.g","subject":"Re: [PATCH v4] alloc: fix dangling pointer in alloc_state cleanup","fromName":"ノウラ | Flare","fromEmail":"nouraellm@gmail.com","sentAt":"2025-09-04T13:25:57Z","receivedAt":"2025-09-04T13:26:01Z","isPatch":true,"sender":{"key":"nouraellm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26191219?v=4"},"body":"The point of the check was to avoid subtle crashes\naligning with defensive programming requirements.\n\nIf you lean more towards strict contract enforcement\nJust say the word.\n\nOn 04/09/2025 09:47, Junio C Hamano wrote:\n> \"ノウラ | Flare via GitGitGadget\" writes:\n>\n>> +void alloc_state_free_and_null(struct alloc_state **s_)\n>> {\n>> +\tstruct alloc_state *s;\n>> +\n>> +\tif (!s_ || !*s_) return;\n> I still do not see the point of this check. If the caller passes a\n> NULL pointer, when they are expected to pass the address of a\n> pointer variable so that the struct the pointer points at is cleared\n> and freed, and the pointer variable is NULLed, it is called a\n> programmer error and they deserve a segfault. Why would it be\n> better to sweep such an error under the rug by returning without\n> anything? It would delay discovery of such a bug, but for what\n> gain?\n>\n"},{"id":"525548","messageId":"xmqqms7a9niw.fsf@gitster.g","threadId":"64041","inReplyTo":"c8137063-c928-45a1-aae5-9de2b35e5150@gmail.com","subject":"Re: [PATCH v4] alloc: fix dangling pointer in alloc_state cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-04T16:43:19Z","receivedAt":"2025-09-04T16:43:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ノウラ | Flare writes:\n\n> The point of the check was to avoid subtle crashes\n> aligning with defensive programming requirements.\n\nIt is a programming error, period. Do not silently return. That's\nnot being defensive. That is sweeping a problem under the rug.\n\nWe can always do assert() or if (condition) BUG(\"...\") if we are\nunsure about the ability of our developers to grok subtle\npre-conditions to call into certain code paths, and I would call\nthat defensive.\n\nBut I do not see a need for _this_ particular call. Our developers\nshould be able to see what they are supposed to call it with just\nfine.\n\nThanks.\n"},{"id":"525554","messageId":"pull.2040.v5.git.git.1757007856062.gitgitgadget@gmail.com","threadId":"64041","inReplyTo":"pull.2040.v4.git.git.1756941427825.gitgitgadget@gmail.com","subject":"[PATCH v5] alloc: fix dangling pointer in alloc_state cleanup","fromName":"ノウラ | Flare via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-09-04T17:44:16Z","receivedAt":"2025-09-04T17:44:18Z","isPatch":true,"sender":{"key":"nouraellm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26191219?v=4"},"body":"From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=\n \n\nAll callers of clear_alloc_state() immediately free what they\ncleared, so currently it does not hurt anybody that the\nalloc_state is left in an unreusable state, but it is an\nerror-prone API. Replace it with a new function that clears but\nin addition frees the structure, as well as NULLing the pointer\nthat points at it and adjust existing callers.\n\nWhile at it, rename allocate_alloc_state() and name the new\nfunction alloc_state_free_and_null(), to follow more closely the\nfunction naming convention specified in the CodingGuidelines\n(namely, functions about S are named with S_ prefix and then\nverb).\n\nSigned-off-by: ノウラ | Flare \n---\n alloc: fix dangling pointer in alloc_state cleanup\n \n cc: Torsten Bögershausen tboegi@web.de cc: Jeff King peff@peff.net\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v5\nPull-Request: https://github.com/git/git/pull/2040\n\nRange-diff vs v4:\n\n 1: 82d228661d ! 1: d04b0258f5 alloc: fix dangling pointer in alloc_state cleanup\n @@ alloc.c: struct alloc_state {\n -void clear_alloc_state(struct alloc_state *s)\n +void alloc_state_free_and_null(struct alloc_state **s_)\n {\n -+\tstruct alloc_state *s;\n -+\n -+\tif (!s_ || !*s_) return;\n -+\n -+\ts = *s_;\n ++\tstruct alloc_state *s = *s_;\n +\n \twhile (s->slab_nr > 0) {\n \t\ts->slab_nr--;\n\n\n alloc.c | 7 +++++--\n alloc.h | 4 ++--\n object.c | 26 ++++++++++----------------\n 3 files changed, 17 insertions(+), 20 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex 377e80f5dd..3a5d0b2bd8 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -36,19 +36,22 @@ struct alloc_state {\n \tint slab_nr, slab_alloc;\n };\n \n-struct alloc_state *allocate_alloc_state(void)\n+struct alloc_state *alloc_state_alloc(void)\n {\n \treturn xcalloc(1, sizeof(struct alloc_state));\n }\n \n-void clear_alloc_state(struct alloc_state *s)\n+void alloc_state_free_and_null(struct alloc_state **s_)\n {\n+\tstruct alloc_state *s = *s_;\n+\n \twhile (s->slab_nr > 0) {\n \t\ts->slab_nr--;\n \t\tfree(s->slabs[s->slab_nr]);\n \t}\n \n \tFREE_AND_NULL(s->slabs);\n+\tFREE_AND_NULL(*s_);\n }\n \n static inline void *alloc_node(struct alloc_state *s, size_t node_size)\ndiff --git a/alloc.h b/alloc.h\nindex 3f4a0ad310..87a47a9709 100644\n--- a/alloc.h\n+++ b/alloc.h\n@@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);\n void *alloc_tag_node(struct repository *r);\n void *alloc_object_node(struct repository *r);\n \n-struct alloc_state *allocate_alloc_state(void);\n-void clear_alloc_state(struct alloc_state *s);\n+struct alloc_state *alloc_state_alloc(void);\n+void alloc_state_free_and_null(struct alloc_state **s_);\n \n #endif\ndiff --git a/object.c b/object.c\nindex c1553ee433..986114a6db 100644\n--- a/object.c\n+++ b/object.c\n@@ -517,12 +517,11 @@ struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)\n \tmemset(o, 0, sizeof(*o));\n \n \to->repo = repo;\n-\to->blob_state = allocate_alloc_state();\n-\to->tree_state = allocate_alloc_state();\n-\to->commit_state = allocate_alloc_state();\n-\to->tag_state = allocate_alloc_state();\n-\to->object_state = allocate_alloc_state();\n-\n+\to->blob_state = alloc_state_alloc();\n+\to->tree_state = alloc_state_alloc();\n+\to->commit_state = alloc_state_alloc();\n+\to->tag_state = alloc_state_alloc();\n+\to->object_state = alloc_state_alloc();\n \to->is_shallow = -1;\n \tCALLOC_ARRAY(o->shallow_stat, 1);\n \n@@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)\n \to->buffer_slab = NULL;\n \n \tparsed_object_pool_reset_commit_grafts(o);\n-\tclear_alloc_state(o->blob_state);\n-\tclear_alloc_state(o->tree_state);\n-\tclear_alloc_state(o->commit_state);\n-\tclear_alloc_state(o->tag_state);\n-\tclear_alloc_state(o->object_state);\n+\talloc_state_free_and_null(&o->blob_state);\n+\talloc_state_free_and_null(&o->tree_state);\n+\talloc_state_free_and_null(&o->commit_state);\n+\talloc_state_free_and_null(&o->tag_state);\n+\talloc_state_free_and_null(&o->object_state);\n \tstat_validity_clear(o->shallow_stat);\n-\tFREE_AND_NULL(o->blob_state);\n-\tFREE_AND_NULL(o->tree_state);\n-\tFREE_AND_NULL(o->commit_state);\n-\tFREE_AND_NULL(o->tag_state);\n-\tFREE_AND_NULL(o->object_state);\n \tFREE_AND_NULL(o->shallow_stat);\n }\n\nbase-commit: f814da676ae46aac5be0a98b99373a76dee6cedb\n-- \ngitgitgadget\n"},{"id":"525563","messageId":"xmqqqzwm6k45.fsf@gitster.g","threadId":"64041","inReplyTo":"pull.2040.v5.git.git.1757007856062.gitgitgadget@gmail.com","subject":"Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-04T20:25:14Z","receivedAt":"2025-09-04T20:25:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ノウラ | Flare via GitGitGadget\" writes:\n\n> index 377e80f5dd..3a5d0b2bd8 100644\n> --- a/alloc.c\n> +++ b/alloc.c\n> @@ -36,19 +36,22 @@ struct alloc_state {\n> \tint slab_nr, slab_alloc;\n> };\n> \n> -struct alloc_state *allocate_alloc_state(void)\n> +struct alloc_state *alloc_state_alloc(void)\n> {\n> \treturn xcalloc(1, sizeof(struct alloc_state));\n> }\n> \n> -void clear_alloc_state(struct alloc_state *s)\n> +void alloc_state_free_and_null(struct alloc_state **s_)\n> {\n> +\tstruct alloc_state *s = *s_;\n> +\n> \twhile (s->slab_nr > 0) {\n> \t\ts->slab_nr--;\n> \t\tfree(s->slabs[s->slab_nr]);\n> \t}\n> \n> \tFREE_AND_NULL(s->slabs);\n> +\tFREE_AND_NULL(*s_);\n> }\n> \n> static inline void *alloc_node(struct alloc_state *s, size_t node_size)\n\nLooking good.\n\n> diff --git a/alloc.h b/alloc.h\n> index 3f4a0ad310..87a47a9709 100644\n> --- a/alloc.h\n> +++ b/alloc.h\n> @@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);\n> void *alloc_tag_node(struct repository *r);\n> void *alloc_object_node(struct repository *r);\n> \n> -struct alloc_state *allocate_alloc_state(void);\n> -void clear_alloc_state(struct alloc_state *s);\n> +struct alloc_state *alloc_state_alloc(void);\n> +void alloc_state_free_and_null(struct alloc_state **s_);\n> \n> #endif\n> diff --git a/object.c b/object.c\n> index c1553ee433..986114a6db 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)\n> \to->buffer_slab = NULL;\n> \n> \tparsed_object_pool_reset_commit_grafts(o);\n> -\tclear_alloc_state(o->blob_state);\n> -\tclear_alloc_state(o->tree_state);\n> -\tclear_alloc_state(o->commit_state);\n> -\tclear_alloc_state(o->tag_state);\n> -\tclear_alloc_state(o->object_state);\n> +\talloc_state_free_and_null(&o->blob_state);\n> +\talloc_state_free_and_null(&o->tree_state);\n> +\talloc_state_free_and_null(&o->commit_state);\n> +\talloc_state_free_and_null(&o->tag_state);\n> +\talloc_state_free_and_null(&o->object_state);\n> \tstat_validity_clear(o->shallow_stat);\n> -\tFREE_AND_NULL(o->blob_state);\n> -\tFREE_AND_NULL(o->tree_state);\n> -\tFREE_AND_NULL(o->commit_state);\n> -\tFREE_AND_NULL(o->tag_state);\n> -\tFREE_AND_NULL(o->object_state);\n> \tFREE_AND_NULL(o->shallow_stat);\n> }\n\nVery nice.\n\nThanks. Will queue.\n\n"},{"id":"525564","messageId":"20250904204932.GD30633@coredump.intra.peff.net","threadId":"64041","inReplyTo":"pull.2040.v5.git.git.1757007856062.gitgitgadget@gmail.com","subject":"Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-04T20:49:32Z","receivedAt":"2025-09-04T20:49:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 04, 2025 at 05:44:16PM +0000, ノウラ | Flare via GitGitGadget wrote:\n\n> -void clear_alloc_state(struct alloc_state *s)\n> +void alloc_state_free_and_null(struct alloc_state **s_)\n> {\n> +\tstruct alloc_state *s = *s_;\n> +\n> \twhile (s->slab_nr > 0) {\n> \t\ts->slab_nr--;\n> \t\tfree(s->slabs[s->slab_nr]);\n> \t}\n> \n> \tFREE_AND_NULL(s->slabs);\n> +\tFREE_AND_NULL(*s_);\n> }\n\nIt's probably not worth going back and forth on this too much, but I\nthought the happy medium was:\n\n if (!s)\n\treturn;\n\nThat is, it is perfectly reasonable and friendly for it to be a noop to\nfree-and-null a NULL value (either never initialized, or already freed).\nThe overkill was worrying about whether somebody passed in a NULL\ndouble-pointer. I.e., doing:\n\n alloc_state_free_and_null(&foo);\n\nis reasonable and should be idempotent but:\n\n alloc_state_free_and_null(NULL);\n\nis a silly programming error that we do not need to protect against.\n\n-Peff\n"},{"id":"525567","messageId":"xmqqjz2d7t2q.fsf@gitster.g","threadId":"64041","inReplyTo":"20250904204932.GD30633@coredump.intra.peff.net","subject":"Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-04T22:26:21Z","receivedAt":"2025-09-04T22:26:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It's probably not worth going back and forth on this too much, but I\n> thought the happy medium was:\n>\n> if (!s)\n> \treturn;\n>\n> That is, it is perfectly reasonable and friendly for it to be a noop to\n> free-and-null a NULL value (either never initialized, or already freed).\n> The overkill was worrying about whether somebody passed in a NULL\n> double-pointer. I.e., doing:\n>\n> alloc_state_free_and_null(&foo);\n>\n> is reasonable and should be idempotent\n\n... when foo == NULL, e.g., after alloc_state_free_and_null(&foo)\nhas just successfully returned?\n\nI can by that argument with the reasoning in the updated log message\nbelow. Does it good to everybody?\n\nThanks.\n\n--- >8 ---\nFrom: ノウラ | Flare \nSubject: [PATCH] alloc: fix dangling pointer in alloc_state cleanup\n\nAll callers of clear_alloc_state() immediately free what they\ncleared, so currently it does not hurt anybody that the\nalloc_state is left in an unreusable state, but it is an\nerror-prone API. Replace it with a new function that clears but\nin addition frees the structure, as well as NULLing the pointer\nthat points at it and adjust existing callers.\n\nAs it is a moral equivalent of FREE_AND_NULL(), except that what it\nfrees has internal structure that needs to be cleaned, allow the\nhelper to be called twice in a row, by making a call with a pointer\nto a pointer variable that already is NULLed.\n\nWhile at it, rename allocate_alloc_state() and name the new\nfunction alloc_state_free_and_null(), to follow more closely the\nfunction naming convention specified in the CodingGuidelines\n(namely, functions about S are named with S_ prefix and then\nverb).\n\nSigned-off-by: ノウラ | Flare \nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n alloc.c | 10 ++++++++--\n alloc.h | 4 ++--\n object.c | 26 ++++++++++----------------\n 3 files changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex 377e80f5dd..533a045c2a 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -36,19 +36,25 @@ struct alloc_state {\n \tint slab_nr, slab_alloc;\n };\n \n-struct alloc_state *allocate_alloc_state(void)\n+struct alloc_state *alloc_state_alloc(void)\n {\n \treturn xcalloc(1, sizeof(struct alloc_state));\n }\n \n-void clear_alloc_state(struct alloc_state *s)\n+void alloc_state_free_and_null(struct alloc_state **s_)\n {\n+\tstruct alloc_state *s = *s_;\n+\n+\tif (!s)\n+\t\treturn;\n+\n \twhile (s->slab_nr > 0) {\n \t\ts->slab_nr--;\n \t\tfree(s->slabs[s->slab_nr]);\n \t}\n \n \tFREE_AND_NULL(s->slabs);\n+\tFREE_AND_NULL(*s_);\n }\n \n static inline void *alloc_node(struct alloc_state *s, size_t node_size)\ndiff --git a/alloc.h b/alloc.h\nindex 3f4a0ad310..87a47a9709 100644\n--- a/alloc.h\n+++ b/alloc.h\n@@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);\n void *alloc_tag_node(struct repository *r);\n void *alloc_object_node(struct repository *r);\n \n-struct alloc_state *allocate_alloc_state(void);\n-void clear_alloc_state(struct alloc_state *s);\n+struct alloc_state *alloc_state_alloc(void);\n+void alloc_state_free_and_null(struct alloc_state **s_);\n \n #endif\ndiff --git a/object.c b/object.c\nindex c1553ee433..986114a6db 100644\n--- a/object.c\n+++ b/object.c\n@@ -517,12 +517,11 @@ struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)\n \tmemset(o, 0, sizeof(*o));\n \n \to->repo = repo;\n-\to->blob_state = allocate_alloc_state();\n-\to->tree_state = allocate_alloc_state();\n-\to->commit_state = allocate_alloc_state();\n-\to->tag_state = allocate_alloc_state();\n-\to->object_state = allocate_alloc_state();\n-\n+\to->blob_state = alloc_state_alloc();\n+\to->tree_state = alloc_state_alloc();\n+\to->commit_state = alloc_state_alloc();\n+\to->tag_state = alloc_state_alloc();\n+\to->object_state = alloc_state_alloc();\n \to->is_shallow = -1;\n \tCALLOC_ARRAY(o->shallow_stat, 1);\n \n@@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)\n \to->buffer_slab = NULL;\n \n \tparsed_object_pool_reset_commit_grafts(o);\n-\tclear_alloc_state(o->blob_state);\n-\tclear_alloc_state(o->tree_state);\n-\tclear_alloc_state(o->commit_state);\n-\tclear_alloc_state(o->tag_state);\n-\tclear_alloc_state(o->object_state);\n+\talloc_state_free_and_null(&o->blob_state);\n+\talloc_state_free_and_null(&o->tree_state);\n+\talloc_state_free_and_null(&o->commit_state);\n+\talloc_state_free_and_null(&o->tag_state);\n+\talloc_state_free_and_null(&o->object_state);\n \tstat_validity_clear(o->shallow_stat);\n-\tFREE_AND_NULL(o->blob_state);\n-\tFREE_AND_NULL(o->tree_state);\n-\tFREE_AND_NULL(o->commit_state);\n-\tFREE_AND_NULL(o->tag_state);\n-\tFREE_AND_NULL(o->object_state);\n \tFREE_AND_NULL(o->shallow_stat);\n }\n-- \n2.51.0-314-g9a15cfd6dc\n\n\n"},{"id":"525577","messageId":"8f831259-7372-4357-b059-cc21f7a04864@gmail.com","threadId":"64041","inReplyTo":"xmqqjz2d7t2q.fsf@gitster.g","subject":"Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup","fromName":"ノウラ | Flare","fromEmail":"nouraellm@gmail.com","sentAt":"2025-09-05T00:02:39Z","receivedAt":"2025-09-05T00:02:42Z","isPatch":true,"sender":{"key":"nouraellm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26191219?v=4"},"body":"No. I am confused here.\n\n > It is a programming error, period.  Do not silently return. That's\n > not being defensive.  That is sweeping a problem under the rug.\n\nYet\n\n > +    if (!s) return;\n\nHowever, I agree with Peff. After calling alloc_state_free_and_null(&foo)\nHaving foo == NULL is an expected behavior, especially since the function\nIs designed to free the memory and null out the caller’s pointer using\nA double pointer ensuring the helper is idempotent\n\nSo, calling it again on the same pointer is safe because it simply\nno-ops if the memory is already freed\n\nRegarding the sanity check, it should be:\n\n+ if (!*s) return;\n\nThe reasoning is the following:\n\n=> s is the double pointer (the address of the caller’s pointer)\n=> *s is the actual pointer to the memory we want to free\n\nThus, we check !*s to allow the function to safely handle already-NULL \npointers\nBut if we instead checked !s then we would be testing whether the caller \npassed\nA NULL double pointer which is a programmer error\nSo silently returning on !s would hide a bug\n\nFinally, I'd appreciate more explicit instructions.\n\nOn 05/09/2025 00:26, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> It's probably not worth going back and forth on this too much, but I\n>> thought the happy medium was:\n>>\n>> if (!s)\n>> \treturn;\n>>\n>> That is, it is perfectly reasonable and friendly for it to be a noop to\n>> free-and-null a NULL value (either never initialized, or already freed).\n>> The overkill was worrying about whether somebody passed in a NULL\n>> double-pointer. I.e., doing:\n>>\n>> alloc_state_free_and_null(&foo);\n>>\n>> is reasonable and should be idempotent\n> ... when foo == NULL, e.g., after alloc_state_free_and_null(&foo)\n> has just successfully returned?\n>\n> I can by that argument with the reasoning in the updated log message\n> below. Does it good to everybody?\n>\n> Thanks.\n>\n> --- >8 ---\n> From: ノウラ | Flare \n> Subject: [PATCH] alloc: fix dangling pointer in alloc_state cleanup\n>\n> All callers of clear_alloc_state() immediately free what they\n> cleared, so currently it does not hurt anybody that the\n> alloc_state is left in an unreusable state, but it is an\n> error-prone API. Replace it with a new function that clears but\n> in addition frees the structure, as well as NULLing the pointer\n> that points at it and adjust existing callers.\n>\n> As it is a moral equivalent of FREE_AND_NULL(), except that what it\n> frees has internal structure that needs to be cleaned, allow the\n> helper to be called twice in a row, by making a call with a pointer\n> to a pointer variable that already is NULLed.\n>\n> While at it, rename allocate_alloc_state() and name the new\n> function alloc_state_free_and_null(), to follow more closely the\n> function naming convention specified in the CodingGuidelines\n> (namely, functions about S are named with S_ prefix and then\n> verb).\n>\n> Signed-off-by: ノウラ | Flare \n> Helped-by: Jeff King \n> Signed-off-by: Junio C Hamano \n> ---\n> alloc.c | 10 ++++++++--\n> alloc.h | 4 ++--\n> object.c | 26 ++++++++++----------------\n> 3 files changed, 20 insertions(+), 20 deletions(-)\n>\n> diff --git a/alloc.c b/alloc.c\n> index 377e80f5dd..533a045c2a 100644\n> --- a/alloc.c\n> +++ b/alloc.c\n> @@ -36,19 +36,25 @@ struct alloc_state {\n> \tint slab_nr, slab_alloc;\n> };\n> \n> -struct alloc_state *allocate_alloc_state(void)\n> +struct alloc_state *alloc_state_alloc(void)\n> {\n> \treturn xcalloc(1, sizeof(struct alloc_state));\n> }\n> \n> -void clear_alloc_state(struct alloc_state *s)\n> +void alloc_state_free_and_null(struct alloc_state **s_)\n> {\n> +\tstruct alloc_state *s = *s_;\n> +\n> +\tif (!s)\n> +\t\treturn;\n> +\n> \twhile (s->slab_nr > 0) {\n> \t\ts->slab_nr--;\n> \t\tfree(s->slabs[s->slab_nr]);\n> \t}\n> \n> \tFREE_AND_NULL(s->slabs);\n> +\tFREE_AND_NULL(*s_);\n> }\n> \n> static inline void *alloc_node(struct alloc_state *s, size_t node_size)\n> diff --git a/alloc.h b/alloc.h\n> index 3f4a0ad310..87a47a9709 100644\n> --- a/alloc.h\n> +++ b/alloc.h\n> @@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);\n> void *alloc_tag_node(struct repository *r);\n> void *alloc_object_node(struct repository *r);\n> \n> -struct alloc_state *allocate_alloc_state(void);\n> -void clear_alloc_state(struct alloc_state *s);\n> +struct alloc_state *alloc_state_alloc(void);\n> +void alloc_state_free_and_null(struct alloc_state **s_);\n> \n> #endif\n> diff --git a/object.c b/object.c\n> index c1553ee433..986114a6db 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -517,12 +517,11 @@ struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)\n> \tmemset(o, 0, sizeof(*o));\n> \n> \to->repo = repo;\n> -\to->blob_state = allocate_alloc_state();\n> -\to->tree_state = allocate_alloc_state();\n> -\to->commit_state = allocate_alloc_state();\n> -\to->tag_state = allocate_alloc_state();\n> -\to->object_state = allocate_alloc_state();\n> -\n> +\to->blob_state = alloc_state_alloc();\n> +\to->tree_state = alloc_state_alloc();\n> +\to->commit_state = alloc_state_alloc();\n> +\to->tag_state = alloc_state_alloc();\n> +\to->object_state = alloc_state_alloc();\n> \to->is_shallow = -1;\n> \tCALLOC_ARRAY(o->shallow_stat, 1);\n> \n> @@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)\n> \to->buffer_slab = NULL;\n> \n> \tparsed_object_pool_reset_commit_grafts(o);\n> -\tclear_alloc_state(o->blob_state);\n> -\tclear_alloc_state(o->tree_state);\n> -\tclear_alloc_state(o->commit_state);\n> -\tclear_alloc_state(o->tag_state);\n> -\tclear_alloc_state(o->object_state);\n> +\talloc_state_free_and_null(&o->blob_state);\n> +\talloc_state_free_and_null(&o->tree_state);\n> +\talloc_state_free_and_null(&o->commit_state);\n> +\talloc_state_free_and_null(&o->tag_state);\n> +\talloc_state_free_and_null(&o->object_state);\n> \tstat_validity_clear(o->shallow_stat);\n> -\tFREE_AND_NULL(o->blob_state);\n> -\tFREE_AND_NULL(o->tree_state);\n> -\tFREE_AND_NULL(o->commit_state);\n> -\tFREE_AND_NULL(o->tag_state);\n> -\tFREE_AND_NULL(o->object_state);\n> \tFREE_AND_NULL(o->shallow_stat);\n> }\n"},{"id":"525578","messageId":"52fb3fea-4842-4532-8572-df41537da5f9@gmail.com","threadId":"64041","inReplyTo":"xmqqjz2d7t2q.fsf@gitster.g","subject":"Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup","fromName":"ノウラ | Flare","fromEmail":"nouraellm@gmail.com","sentAt":"2025-09-05T00:07:10Z","receivedAt":"2025-09-05T00:07:13Z","isPatch":true,"sender":{"key":"nouraellm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26191219?v=4"},"body":"No. I am confused here.\n\n > It is a programming error, period.  Do not silently return. That's\n > not being defensive.  That is sweeping a problem under the rug.\n\nYet\n\n > +    if (!s) return;\n\nHowever, I agree with Peff. After calling alloc_state_free_and_null(&foo)\nHaving foo == NULL is an expected behavior, especially since the function\nIs designed to free the memory and null out the caller’s pointer using\nA double pointer ensuring the helper is idempotent\n\nSo, calling it again on the same pointer is safe because it simply\nno-ops if the memory is already freed\n\nRegarding the sanity check, it should be:\n\n+ if (!*s) return;\n\nThe reasoning is the following:\n\n=> s is the double pointer (the address of the caller’s pointer)\n=> *s is the actual pointer to the memory we want to free\n\nThus, we check !*s to allow the function to safely handle already-NULL \npointers\nBut if we instead checked !s then we would be testing whether the caller \npassed\nA NULL double pointer which is a programmer error\nSo silently returning on !s would hide a bug\n\nFinally, I'd appreciate more explicit instructions.\n\nOn 05/09/2025 00:26, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> It's probably not worth going back and forth on this too much, but I\n>> thought the happy medium was:\n>>\n>> if (!s)\n>> \treturn;\n>>\n>> That is, it is perfectly reasonable and friendly for it to be a noop to\n>> free-and-null a NULL value (either never initialized, or already freed).\n>> The overkill was worrying about whether somebody passed in a NULL\n>> double-pointer. I.e., doing:\n>>\n>> alloc_state_free_and_null(&foo);\n>>\n>> is reasonable and should be idempotent\n> ... when foo == NULL, e.g., after alloc_state_free_and_null(&foo)\n> has just successfully returned?\n>\n> I can by that argument with the reasoning in the updated log message\n> below. Does it good to everybody?\n>\n> Thanks.\n>\n> --- >8 ---\n> From: ノウラ | Flare \n> Subject: [PATCH] alloc: fix dangling pointer in alloc_state cleanup\n>\n> All callers of clear_alloc_state() immediately free what they\n> cleared, so currently it does not hurt anybody that the\n> alloc_state is left in an unreusable state, but it is an\n> error-prone API. Replace it with a new function that clears but\n> in addition frees the structure, as well as NULLing the pointer\n> that points at it and adjust existing callers.\n>\n> As it is a moral equivalent of FREE_AND_NULL(), except that what it\n> frees has internal structure that needs to be cleaned, allow the\n> helper to be called twice in a row, by making a call with a pointer\n> to a pointer variable that already is NULLed.\n>\n> While at it, rename allocate_alloc_state() and name the new\n> function alloc_state_free_and_null(), to follow more closely the\n> function naming convention specified in the CodingGuidelines\n> (namely, functions about S are named with S_ prefix and then\n> verb).\n>\n> Signed-off-by: ノウラ | Flare \n> Helped-by: Jeff King \n> Signed-off-by: Junio C Hamano \n> ---\n> alloc.c | 10 ++++++++--\n> alloc.h | 4 ++--\n> object.c | 26 ++++++++++----------------\n> 3 files changed, 20 insertions(+), 20 deletions(-)\n>\n> diff --git a/alloc.c b/alloc.c\n> index 377e80f5dd..533a045c2a 100644\n> --- a/alloc.c\n> +++ b/alloc.c\n> @@ -36,19 +36,25 @@ struct alloc_state {\n> \tint slab_nr, slab_alloc;\n> };\n> \n> -struct alloc_state *allocate_alloc_state(void)\n> +struct alloc_state *alloc_state_alloc(void)\n> {\n> \treturn xcalloc(1, sizeof(struct alloc_state));\n> }\n> \n> -void clear_alloc_state(struct alloc_state *s)\n> +void alloc_state_free_and_null(struct alloc_state **s_)\n> {\n> +\tstruct alloc_state *s = *s_;\n> +\n> +\tif (!s)\n> +\t\treturn;\n> +\n> \twhile (s->slab_nr > 0) {\n> \t\ts->slab_nr--;\n> \t\tfree(s->slabs[s->slab_nr]);\n> \t}\n> \n> \tFREE_AND_NULL(s->slabs);\n> +\tFREE_AND_NULL(*s_);\n> }\n> \n> static inline void *alloc_node(struct alloc_state *s, size_t node_size)\n> diff --git a/alloc.h b/alloc.h\n> index 3f4a0ad310..87a47a9709 100644\n> --- a/alloc.h\n> +++ b/alloc.h\n> @@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);\n> void *alloc_tag_node(struct repository *r);\n> void *alloc_object_node(struct repository *r);\n> \n> -struct alloc_state *allocate_alloc_state(void);\n> -void clear_alloc_state(struct alloc_state *s);\n> +struct alloc_state *alloc_state_alloc(void);\n> +void alloc_state_free_and_null(struct alloc_state **s_);\n> \n> #endif\n> diff --git a/object.c b/object.c\n> index c1553ee433..986114a6db 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -517,12 +517,11 @@ struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)\n> \tmemset(o, 0, sizeof(*o));\n> \n> \to->repo = repo;\n> -\to->blob_state = allocate_alloc_state();\n> -\to->tree_state = allocate_alloc_state();\n> -\to->commit_state = allocate_alloc_state();\n> -\to->tag_state = allocate_alloc_state();\n> -\to->object_state = allocate_alloc_state();\n> -\n> +\to->blob_state = alloc_state_alloc();\n> +\to->tree_state = alloc_state_alloc();\n> +\to->commit_state = alloc_state_alloc();\n> +\to->tag_state = alloc_state_alloc();\n> +\to->object_state = alloc_state_alloc();\n> \to->is_shallow = -1;\n> \tCALLOC_ARRAY(o->shallow_stat, 1);\n> \n> @@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)\n> \to->buffer_slab = NULL;\n> \n> \tparsed_object_pool_reset_commit_grafts(o);\n> -\tclear_alloc_state(o->blob_state);\n> -\tclear_alloc_state(o->tree_state);\n> -\tclear_alloc_state(o->commit_state);\n> -\tclear_alloc_state(o->tag_state);\n> -\tclear_alloc_state(o->object_state);\n> +\talloc_state_free_and_null(&o->blob_state);\n> +\talloc_state_free_and_null(&o->tree_state);\n> +\talloc_state_free_and_null(&o->commit_state);\n> +\talloc_state_free_and_null(&o->tag_state);\n> +\talloc_state_free_and_null(&o->object_state);\n> \tstat_validity_clear(o->shallow_stat);\n> -\tFREE_AND_NULL(o->blob_state);\n> -\tFREE_AND_NULL(o->tree_state);\n> -\tFREE_AND_NULL(o->commit_state);\n> -\tFREE_AND_NULL(o->tag_state);\n> -\tFREE_AND_NULL(o->object_state);\n> \tFREE_AND_NULL(o->shallow_stat);\n> }\n"},{"id":"525580","messageId":"88ae6c7b-0a69-48bd-9e73-672331a22197@gmail.com","threadId":"64041","inReplyTo":"52fb3fea-4842-4532-8572-df41537da5f9@gmail.com","subject":"Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup","fromName":"ノウラ | Flare","fromEmail":"nouraellm@gmail.com","sentAt":"2025-09-05T00:25:55Z","receivedAt":"2025-09-05T00:25:59Z","isPatch":true,"sender":{"key":"nouraellm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26191219?v=4"},"body":"By *s I am referring to *s_ so a sanity check with: if (!*s_) return;\n\nOn 05/09/2025 02:07, ノウラ | Flare wrote:\n> No. I am confused here.\n>\n> > It is a programming error, period.  Do not silently return. That's\n> > not being defensive.  That is sweeping a problem under the rug.\n>\n> Yet\n>\n> > +    if (!s) return;\n>\n> However, I agree with Peff. After calling alloc_state_free_and_null(&foo)\n> Having foo == NULL is an expected behavior, especially since the function\n> Is designed to free the memory and null out the caller’s pointer using\n> A double pointer ensuring the helper is idempotent\n>\n> So, calling it again on the same pointer is safe because it simply\n> no-ops if the memory is already freed\n>\n> Regarding the sanity check, it should be:\n>\n> + if (!*s) return;\n>\n> The reasoning is the following:\n>\n> => s is the double pointer (the address of the caller’s pointer)\n> => *s is the actual pointer to the memory we want to free\n>\n> Thus, we check !*s to allow the function to safely handle already-NULL \n> pointers\n> But if we instead checked !s then we would be testing whether the \n> caller passed\n> A NULL double pointer which is a programmer error\n> So silently returning on !s would hide a bug\n>\n> Finally, I'd appreciate more explicit instructions.\n>\n> On 05/09/2025 00:26, Junio C Hamano wrote:\n>> Jeff King writes:\n>>\n>>> It's probably not worth going back and forth on this too much, but I\n>>> thought the happy medium was:\n>>>\n>>>    if (!s)\n>>>     return;\n>>>\n>>> That is, it is perfectly reasonable and friendly for it to be a noop to\n>>> free-and-null a NULL value (either never initialized, or already \n>>> freed).\n>>> The overkill was worrying about whether somebody passed in a NULL\n>>> double-pointer. I.e., doing:\n>>>\n>>>    alloc_state_free_and_null(&foo);\n>>>\n>>> is reasonable and should be idempotent\n>> ... when foo == NULL, e.g., after alloc_state_free_and_null(&foo)\n>> has just successfully returned?\n>>\n>> I can by that argument with the reasoning in the updated log message\n>> below.  Does it good to everybody?\n>>\n>> Thanks.\n>>\n>> --- >8 ---\n>> From: ノウラ | Flare \n>> Subject: [PATCH] alloc: fix dangling pointer in alloc_state cleanup\n>>\n>> All callers of clear_alloc_state() immediately free what they\n>> cleared, so currently it does not hurt anybody that the\n>> alloc_state is left in an unreusable state, but it is an\n>> error-prone API. Replace it with a new function that clears but\n>> in addition frees the structure, as well as NULLing the pointer\n>> that points at it and adjust existing callers.\n>>\n>> As it is a moral equivalent of FREE_AND_NULL(), except that what it\n>> frees has internal structure that needs to be cleaned, allow the\n>> helper to be called twice in a row, by making a call with a pointer\n>> to a pointer variable that already is NULLed.\n>>\n>> While at it, rename allocate_alloc_state() and name the new\n>> function alloc_state_free_and_null(), to follow more closely the\n>> function naming convention specified in the CodingGuidelines\n>> (namely, functions about S are named with S_ prefix and then\n>> verb).\n>>\n>> Signed-off-by: ノウラ | Flare \n>> Helped-by: Jeff King \n>> Signed-off-by: Junio C Hamano \n>> ---\n>>   alloc.c  | 10 ++++++++--\n>>   alloc.h  |  4 ++--\n>>   object.c | 26 ++++++++++----------------\n>>   3 files changed, 20 insertions(+), 20 deletions(-)\n>>\n>> diff --git a/alloc.c b/alloc.c\n>> index 377e80f5dd..533a045c2a 100644\n>> --- a/alloc.c\n>> +++ b/alloc.c\n>> @@ -36,19 +36,25 @@ struct alloc_state {\n>>       int slab_nr, slab_alloc;\n>>   };\n>>   -struct alloc_state *allocate_alloc_state(void)\n>> +struct alloc_state *alloc_state_alloc(void)\n>>   {\n>>       return xcalloc(1, sizeof(struct alloc_state));\n>>   }\n>>   -void clear_alloc_state(struct alloc_state *s)\n>> +void alloc_state_free_and_null(struct alloc_state **s_)\n>>   {\n>> +    struct alloc_state *s = *s_;\n>> +\n>> +    if (!s)\n>> +        return;\n>> +\n>>       while (s->slab_nr > 0) {\n>>           s->slab_nr--;\n>>           free(s->slabs[s->slab_nr]);\n>>       }\n>>         FREE_AND_NULL(s->slabs);\n>> +    FREE_AND_NULL(*s_);\n>>   }\n>>     static inline void *alloc_node(struct alloc_state *s, size_t \n>> node_size)\n>> diff --git a/alloc.h b/alloc.h\n>> index 3f4a0ad310..87a47a9709 100644\n>> --- a/alloc.h\n>> +++ b/alloc.h\n>> @@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);\n>>   void *alloc_tag_node(struct repository *r);\n>>   void *alloc_object_node(struct repository *r);\n>>   -struct alloc_state *allocate_alloc_state(void);\n>> -void clear_alloc_state(struct alloc_state *s);\n>> +struct alloc_state *alloc_state_alloc(void);\n>> +void alloc_state_free_and_null(struct alloc_state **s_);\n>>     #endif\n>> diff --git a/object.c b/object.c\n>> index c1553ee433..986114a6db 100644\n>> --- a/object.c\n>> +++ b/object.c\n>> @@ -517,12 +517,11 @@ struct parsed_object_pool \n>> *parsed_object_pool_new(struct repository *repo)\n>>       memset(o, 0, sizeof(*o));\n>>         o->repo = repo;\n>> -    o->blob_state = allocate_alloc_state();\n>> -    o->tree_state = allocate_alloc_state();\n>> -    o->commit_state = allocate_alloc_state();\n>> -    o->tag_state = allocate_alloc_state();\n>> -    o->object_state = allocate_alloc_state();\n>> -\n>> +    o->blob_state = alloc_state_alloc();\n>> +    o->tree_state = alloc_state_alloc();\n>> +    o->commit_state = alloc_state_alloc();\n>> +    o->tag_state = alloc_state_alloc();\n>> +    o->object_state = alloc_state_alloc();\n>>       o->is_shallow = -1;\n>>       CALLOC_ARRAY(o->shallow_stat, 1);\n>>   @@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct \n>> parsed_object_pool *o)\n>>       o->buffer_slab = NULL;\n>>         parsed_object_pool_reset_commit_grafts(o);\n>> -    clear_alloc_state(o->blob_state);\n>> -    clear_alloc_state(o->tree_state);\n>> -    clear_alloc_state(o->commit_state);\n>> -    clear_alloc_state(o->tag_state);\n>> -    clear_alloc_state(o->object_state);\n>> +    alloc_state_free_and_null(&o->blob_state);\n>> +    alloc_state_free_and_null(&o->tree_state);\n>> +    alloc_state_free_and_null(&o->commit_state);\n>> +    alloc_state_free_and_null(&o->tag_state);\n>> +    alloc_state_free_and_null(&o->object_state);\n>>       stat_validity_clear(o->shallow_stat);\n>> -    FREE_AND_NULL(o->blob_state);\n>> -    FREE_AND_NULL(o->tree_state);\n>> -    FREE_AND_NULL(o->commit_state);\n>> -    FREE_AND_NULL(o->tag_state);\n>> -    FREE_AND_NULL(o->object_state);\n>>       FREE_AND_NULL(o->shallow_stat);\n>>   }\n"},{"id":"525581","messageId":"699b387f-b09a-4003-9f6c-5a4a4b527ed5@gmail.com","threadId":"64041","inReplyTo":"88ae6c7b-0a69-48bd-9e73-672331a22197@gmail.com","subject":"Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup","fromName":"ノウラ | Flare","fromEmail":"nouraellm@gmail.com","sentAt":"2025-09-05T01:03:30Z","receivedAt":"2025-09-05T01:03:33Z","isPatch":true,"sender":{"key":"nouraellm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26191219?v=4"},"body":" > => s is the double pointer (the address of the caller’s pointer)\n\nFor this part as well, it's s_ not s\n\n\n\n"},{"id":"525626","messageId":"20250905131513.GB596956@coredump.intra.peff.net","threadId":"64041","inReplyTo":"xmqqjz2d7t2q.fsf@gitster.g","subject":"Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-05T13:15:13Z","receivedAt":"2025-09-05T13:15:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 04, 2025 at 03:26:21PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > It's probably not worth going back and forth on this too much, but I\n> > thought the happy medium was:\n> >\n> > if (!s)\n> > \treturn;\n> >\n> > That is, it is perfectly reasonable and friendly for it to be a noop to\n> > free-and-null a NULL value (either never initialized, or already freed).\n> > The overkill was worrying about whether somebody passed in a NULL\n> > double-pointer. I.e., doing:\n> >\n> > alloc_state_free_and_null(&foo);\n> >\n> > is reasonable and should be idempotent\n> \n> ... when foo == NULL, e.g., after alloc_state_free_and_null(&foo)\n> has just successfully returned?\n\nExactly.\n\n> I can by that argument with the reasoning in the updated log message\n> below. Does it good to everybody?\n\nYep, it looks good to me.\n\n-Peff\n"},{"id":"525629","messageId":"20250905132309.GC596956@coredump.intra.peff.net","threadId":"64041","inReplyTo":"8f831259-7372-4357-b059-cc21f7a04864@gmail.com","subject":"Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-05T13:23:09Z","receivedAt":"2025-09-05T13:23:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 05, 2025 at 02:02:39AM +0200, ノウラ | Flare wrote:\n\n> > +    if (!s) return;\n> \n> However, I agree with Peff. After calling alloc_state_free_and_null(&foo)\n> Having foo == NULL is an expected behavior, especially since the function\n> Is designed to free the memory and null out the caller’s pointer using\n> A double pointer ensuring the helper is idempotent\n> \n> So, calling it again on the same pointer is safe because it simply\n> no-ops if the memory is already freed\n> \n> Regarding the sanity check, it should be:\n> \n> + if (!*s) return;\n\nWith the correction in your follow-up that this should be:\n\n if (!*s_) return;\n\nI agree that is the right thing. But it is equivalent to:\n\n if (!s) return;\n\nsince we'll have just assigned \"s\". Which one to choose is purely a\nmatter of style. Using \"*s_\" perhaps makes it more clear that we are\nsanity-checking the input (and could happen even before we assign \"s\").\nUsing \"s\" is consistent with the rest of the function in working with\nthe more direct pointer value. I am happy with either.\n\n> The reasoning is the following:\n> \n> => s is the double pointer (the address of the caller’s pointer)\n> => *s is the actual pointer to the memory we want to free\n> \n> Thus, we check !*s to allow the function to safely handle already-NULL\n> pointers\n> But if we instead checked !s then we would be testing whether the caller\n> passed\n> A NULL double pointer which is a programmer error\n> So silently returning on !s would hide a bug\n\nI think this is wrong. \"s\" is the value we would have allocated from\nalloc_state_new(), and what we are actually interested in freeing. We\nonly see the double-pointer \"s_\" because we want to modify the caller's\ncontaining pointer.\n\nThe FREE_AND_NULL() macro does not itself have this same confusion\nbecause it _is_ a macro. So it is expanded in the caller's context and\ndoes not need the extra layer of indirection.\n\nI do not think this is a good idea, because it obscures things further\nfor people who are accustomed to C idioms, but one could imagine a more\ngeneral macro like:\n\n #define FREE_AND_NULL_WITH(p, fn) do { fn(p); (p) = NULL; } while (0)\n #define FREE_AND_NULL(p) FREE_AND_NULL_WITH(p, free)\n\nand then you could do:\n\n FREE_AND_NULL_WITH(o->blob_state, alloc_state_free);\n\nwhere alloc_state_free() would only receive the single pointer \"s\", and\nwould free it but not assign NULL.\n\n-Peff\n"},{"id":"525643","messageId":"xmqqecsl55gk.fsf@gitster.g","threadId":"64041","inReplyTo":"88ae6c7b-0a69-48bd-9e73-672331a22197@gmail.com","subject":"Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-05T14:39:23Z","receivedAt":"2025-09-05T14:39:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ノウラ | Flare writes:\n\n> By *s I am referring to *s_ so a sanity check with: if (!*s_) return;\n\nBecause we\n\n\ts = *s_;\n\nupfront, exactly because we do want the code to segfault if the\ncaller passes NULL to the function (so s_ that is NULL will cause a\nNULL dereference right there), after that happens checking the NULL\nness of s and *s_ is equivalent.\n\nAnd the whole point of doing \"s = *s_\" upfront is because readers\ncan easily get confused when they have to deal with double pointers.\nThe only reason why we pass the address of the pointer variable is\nso that we can assign NULL to it at the very end, and before we can\ndo so, we want to be able inspect the innards of alloc_state object.\nBy dereferencing s_ early into s, the code can work with the object\nitself without having to worry about following double pointer, so\neven though if (!*s_) and if (!s) may be equivalent, writing the\nlatter is more in line with the whole reason why we have a variable\n's' that is separate from 's_'.\n"},{"id":"525651","messageId":"4010daa5-92af-4d96-a27b-bcb6eeb5d3a1@gmail.com","threadId":"64041","inReplyTo":"20250905132309.GC596956@coredump.intra.peff.net","subject":"Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup","fromName":"ノウラ | Flare","fromEmail":"nouraellm@gmail.com","sentAt":"2025-09-05T17:27:30Z","receivedAt":"2025-09-05T17:27:33Z","isPatch":true,"sender":{"key":"nouraellm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26191219?v=4"},"body":">>> > I agree that is the right thing. But it is equivalent to:\n>>>\n>>> > if (!s) return;\n>>>\n>>> > since we'll have just assigned \"s\". Which one to choose is purely a\n>>> > matter of style. Using \"*s_\" perhaps makes it more clear that we are\n>>> > sanity-checking the input (and could happen even before we assign \"s\").\n>>> > Using \"s\" is consistent with the rest of the function in working with\n>>> > the more direct pointer value. I am happy with either.\n\nI know, I used the original one (**s_) for the explanation. No the check cannot\nhappen before we assign (that's what I did initially) because tests will\nfail given C90 requires declarations to be at the top of the block.\n\n"},{"id":"525653","messageId":"44ae54a6-ba49-40e7-b519-15c12cca0404@gmail.com","threadId":"64041","inReplyTo":"xmqqecsl55gk.fsf@gitster.g","subject":"Re: [PATCH v5] alloc: fix dangling pointer in alloc_state cleanup","fromName":"ノウラ | Flare","fromEmail":"nouraellm@gmail.com","sentAt":"2025-09-05T17:47:47Z","receivedAt":"2025-09-05T17:47:50Z","isPatch":true,"sender":{"key":"nouraellm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26191219?v=4"},"body":"\nOn 05/09/2025 16:39, Junio C Hamano wrote:\n> ノウラ | Flare writes:\n>\n>> By *s I am referring to *s_ so a sanity check with: if (!*s_) return;\n> Because we\n>\n> \ts = *s_;\n>\n> upfront, exactly because we do want the code to segfault if the\n> caller passes NULL to the function (so s_ that is NULL will cause a\n> NULL dereference right there), after that happens checking the NULL\n> ness of s and *s_ is equivalent.\n>\n> And the whole point of doing \"s = *s_\" upfront is because readers\n> can easily get confused when they have to deal with double pointers.\n> The only reason why we pass the address of the pointer variable is\n> so that we can assign NULL to it at the very end, and before we can\n> do so, we want to be able inspect the innards of alloc_state object.\n> By dereferencing s_ early into s, the code can work with the object\n> itself without having to worry about following double pointer, so\n> even though if (!*s_) and if (!s) may be equivalent, writing the\n> latter is more in line with the whole reason why we have a variable\n> 's' that is separate from 's_'.\n\nI see your point, but I previously wrote we could lean more towards\n\nStrict contract enforcement instead of defensive programming\n\nWhich would keep if (!s) return; instead of if (!s_ || !*s_) return;\n\nIn any case, I believe we all now agree on if (!s) return;\n\nPushing it right now.\n\nHappy w-e y'all!\n\n"},{"id":"525657","messageId":"pull.2040.v6.git.git.1757098308277.gitgitgadget@gmail.com","threadId":"64041","inReplyTo":"pull.2040.v5.git.git.1757007856062.gitgitgadget@gmail.com","subject":"[PATCH v6] alloc: fix dangling pointer in alloc_state cleanup","fromName":"ノウラ | Flare via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-09-05T18:51:48Z","receivedAt":"2025-09-05T18:51:51Z","isPatch":true,"sender":{"key":"nouraellm@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26191219?v=4"},"body":"From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=\n \n\nAll callers of clear_alloc_state() immediately free what they\ncleared, so currently it does not hurt anybody that the\nalloc_state is left in an unreusable state, but it is an\nerror-prone API. Replace it with a new function that clears but\nin addition frees the structure, as well as NULLing the pointer\nthat points at it and adjust existing callers.\n\nAs it is a moral equivalent of FREE_AND_NULL(), except that what it\nfrees has internal structure that needs to be cleaned, allow the\nhelper to be called twice in a row, by making a call with a pointer\nto a pointer variable that already is NULLed.\n\nWhile at it, rename allocate_alloc_state() and name the new\nfunction alloc_state_free_and_null(), to follow more closely the\nfunction naming convention specified in the CodingGuidelines\n(namely, functions about S are named with S_ prefix and then\nverb).\n\nSigned-off-by: ノウラ | Flare \nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n alloc: fix dangling pointer in alloc_state cleanup\n \n cc: Torsten Bögershausen tboegi@web.de cc: Jeff King peff@peff.net\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v6\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v6\nPull-Request: https://github.com/git/git/pull/2040\n\nRange-diff vs v5:\n\n 1: d04b0258f5 ! 1: 5afd5b6a26 alloc: fix dangling pointer in alloc_state cleanup\n @@ Commit message\n in addition frees the structure, as well as NULLing the pointer\n that points at it and adjust existing callers.\n \n + As it is a moral equivalent of FREE_AND_NULL(), except that what it\n + frees has internal structure that needs to be cleaned, allow the\n + helper to be called twice in a row, by making a call with a pointer\n + to a pointer variable that already is NULLed.\n +\n While at it, rename allocate_alloc_state() and name the new\n function alloc_state_free_and_null(), to follow more closely the\n function naming convention specified in the CodingGuidelines\n @@ Commit message\n verb).\n \n Signed-off-by: ノウラ | Flare \n + Helped-by: Jeff King \n + Signed-off-by: Junio C Hamano \n \n ## alloc.c ##\n @@ alloc.c: struct alloc_state {\n @@ alloc.c: struct alloc_state {\n +void alloc_state_free_and_null(struct alloc_state **s_)\n {\n +\tstruct alloc_state *s = *s_;\n ++\n ++\tif (!s)\n ++\t return;\n +\n \twhile (s->slab_nr > 0) {\n \t\ts->slab_nr--;\n\n\n alloc.c | 10 ++++++++--\n alloc.h | 4 ++--\n object.c | 26 ++++++++++----------------\n 3 files changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/alloc.c b/alloc.c\nindex 377e80f5dd..7a0af6b4bb 100644\n--- a/alloc.c\n+++ b/alloc.c\n@@ -36,19 +36,25 @@ struct alloc_state {\n \tint slab_nr, slab_alloc;\n };\n \n-struct alloc_state *allocate_alloc_state(void)\n+struct alloc_state *alloc_state_alloc(void)\n {\n \treturn xcalloc(1, sizeof(struct alloc_state));\n }\n \n-void clear_alloc_state(struct alloc_state *s)\n+void alloc_state_free_and_null(struct alloc_state **s_)\n {\n+\tstruct alloc_state *s = *s_;\n+\n+\tif (!s)\n+\t return;\n+\n \twhile (s->slab_nr > 0) {\n \t\ts->slab_nr--;\n \t\tfree(s->slabs[s->slab_nr]);\n \t}\n \n \tFREE_AND_NULL(s->slabs);\n+\tFREE_AND_NULL(*s_);\n }\n \n static inline void *alloc_node(struct alloc_state *s, size_t node_size)\ndiff --git a/alloc.h b/alloc.h\nindex 3f4a0ad310..87a47a9709 100644\n--- a/alloc.h\n+++ b/alloc.h\n@@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);\n void *alloc_tag_node(struct repository *r);\n void *alloc_object_node(struct repository *r);\n \n-struct alloc_state *allocate_alloc_state(void);\n-void clear_alloc_state(struct alloc_state *s);\n+struct alloc_state *alloc_state_alloc(void);\n+void alloc_state_free_and_null(struct alloc_state **s_);\n \n #endif\ndiff --git a/object.c b/object.c\nindex c1553ee433..986114a6db 100644\n--- a/object.c\n+++ b/object.c\n@@ -517,12 +517,11 @@ struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)\n \tmemset(o, 0, sizeof(*o));\n \n \to->repo = repo;\n-\to->blob_state = allocate_alloc_state();\n-\to->tree_state = allocate_alloc_state();\n-\to->commit_state = allocate_alloc_state();\n-\to->tag_state = allocate_alloc_state();\n-\to->object_state = allocate_alloc_state();\n-\n+\to->blob_state = alloc_state_alloc();\n+\to->tree_state = alloc_state_alloc();\n+\to->commit_state = alloc_state_alloc();\n+\to->tag_state = alloc_state_alloc();\n+\to->object_state = alloc_state_alloc();\n \to->is_shallow = -1;\n \tCALLOC_ARRAY(o->shallow_stat, 1);\n \n@@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)\n \to->buffer_slab = NULL;\n \n \tparsed_object_pool_reset_commit_grafts(o);\n-\tclear_alloc_state(o->blob_state);\n-\tclear_alloc_state(o->tree_state);\n-\tclear_alloc_state(o->commit_state);\n-\tclear_alloc_state(o->tag_state);\n-\tclear_alloc_state(o->object_state);\n+\talloc_state_free_and_null(&o->blob_state);\n+\talloc_state_free_and_null(&o->tree_state);\n+\talloc_state_free_and_null(&o->commit_state);\n+\talloc_state_free_and_null(&o->tag_state);\n+\talloc_state_free_and_null(&o->object_state);\n \tstat_validity_clear(o->shallow_stat);\n-\tFREE_AND_NULL(o->blob_state);\n-\tFREE_AND_NULL(o->tree_state);\n-\tFREE_AND_NULL(o->commit_state);\n-\tFREE_AND_NULL(o->tag_state);\n-\tFREE_AND_NULL(o->object_state);\n \tFREE_AND_NULL(o->shallow_stat);\n }\n\nbase-commit: f814da676ae46aac5be0a98b99373a76dee6cedb\n-- \ngitgitgadget\n"},{"id":"525669","messageId":"xmqqh5xg3d3a.fsf@gitster.g","threadId":"64041","inReplyTo":"pull.2040.v6.git.git.1757098308277.gitgitgadget@gmail.com","subject":"Re: [PATCH v6] alloc: fix dangling pointer in alloc_state cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-05T19:37:29Z","receivedAt":"2025-09-05T19:37:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ノウラ | Flare via GitGitGadget\" writes:\n\n> From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=\n> \n>\n> All callers of clear_alloc_state() immediately free what they\n> cleared, so currently it does not hurt anybody that the\n> alloc_state is left in an unreusable state, but it is an\n> error-prone API. Replace it with a new function that clears but\n> in addition frees the structure, as well as NULLing the pointer\n> that points at it and adjust existing callers.\n>\n> As it is a moral equivalent of FREE_AND_NULL(), except that what it\n> frees has internal structure that needs to be cleaned, allow the\n> helper to be called twice in a row, by making a call with a pointer\n> to a pointer variable that already is NULLed.\n>\n> While at it, rename allocate_alloc_state() and name the new\n> function alloc_state_free_and_null(), to follow more closely the\n> function naming convention specified in the CodingGuidelines\n> (namely, functions about S are named with S_ prefix and then\n> verb).\n>\n> Signed-off-by: ノウラ | Flare \n> Helped-by: Jeff King \n> Signed-off-by: Junio C Hamano \n> ---\n> alloc: fix dangling pointer in alloc_state cleanup\n> \n> cc: Torsten Bögershausen tboegi@web.de cc: Jeff King peff@peff.net\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v6\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v6\n> Pull-Request: https://github.com/git/git/pull/2040\n\nNicely done.\nI see no more need for further changes.\n\n> +\tif (!s)\n> +\t return;\n\nThis must be indented with two tabs, i.e.\n\n\tif (!s)\n\t\treturn;\n\nbut the copy I have since yesterday already is formatted that way,\nso no need to resend this patch.\n\nThanks. Let's mark the topic for 'next'.\n"}]}