reset slab_alloc and state fields in clear_alloc_state()

From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9?= <[email protected]>

clear_alloc_state() freed all slabs and nulled the slabs pointer but left slab_alloc, nr, and p unchanged. If the alloc_state is reused, ALLOC_GROW() can wrongly assume that the slab array is already allocated because slab_alloc still holds a stale nonzero capacity. In that case s->slabs remains NULL and the next dereference writes through a NULL pointer, causing undefined behavior.

From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=
<[email protected]>

clear_alloc_state() freed all slabs and nulled the slabs pointer but left slab_alloc, nr, and p unchanged. If the alloc_state is reused, ALLOC_GROW() can wrongly assume that the slab array is already allocated because slab_alloc still holds a stale nonzero capacity. In that case s->slabs remains NULL and the next dereference writes through a NULL pointer, causing undefined behavior.

To fix this, this patch:

- Renames allocate_alloc_state() → alloc_state_alloc().
- Replaces the “just clear” API with alloc_state_free_and_null(struct alloc_state **s_), which frees all slabs and the alloc_state itself, and then nulls the caller’s pointer.
- Updates call sites to use the new helpers and drops redundant FREE_AND_NULL() calls.

This makes the alloc_state lifecycle API harder to misuse, eliminates stale-capacity state,
and aligns naming with project conventions.

Quoted 9 lines · ノウラ | Flare via GitGitGadget

From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=
<[email protected]>

clear_alloc_state() freed all slabs and nulled the slabs pointer but left slab_alloc, nr, and p unchanged. If the alloc_state is reused, ALLOC_GROW() can wrongly assume that the slab array is already allocated because slab_alloc still holds a stale nonzero capacity. In that case s->slabs remains NULL and the next dereference writes through a NULL pointer, causing undefined behavior.

This is good.

Quoted 1 line

Quoted 1 line

To fix this, this patch:

Style nit, we tend to use the "imperative form" here in Git, like this:

- Rename allocate_alloc_state() → alloc_state_alloc().
- Replace ...
- Update ...

Quoted 6 lines

Style nit, we tend to use the "imperative form" here in Git, like this:

- Rename allocate_alloc_state() → alloc_state_alloc().
- Replace ...
- Update ...

Thanks. We also tend to avoid bulleted list.

alloc.c struct alloc_state { · line 49

Quoted 6 lines · ノウラ | Flare via GitGitGadget

alloc.c +5 -0

+void alloc_state_free_and_null(struct alloc_state **s_)
 {
+	struct alloc_state *s = *s_;
+
+	if (!s_ || !*s_) return;
+

Coverity complains that there's a NULL check here for "s_", but we'll have already dereferenced it in the initializer for "s".

I don't think any caller passes NULL, so you can't trigger a segfault in practice. But the code is kind of misleading. Should it just be:

if (!*s_)

	return;

? Or even just "if (!s)".

alloc.c struct alloc_state { · line 48

Quoted 5 lines · ノウラ | Flare via GitGitGadget

alloc.c +4 -0

+void alloc_state_free_and_null(struct alloc_state **s_)
  {
+	struct alloc_state *s;
+
+	if (!s_ || !*s_) return;

I still do not see the point of this check. If the caller passes a NULL pointer, when they are expected to pass the address of a pointer variable so that the struct the pointer points at is cleared and freed, and the pointer variable is NULLed, it is called a programmer error and they deserve a segfault. Why would it be better to sweep such an error under the rug by returning without anything? It would delay discovery of such a bug, but for what gain?

The point of the check was to avoid subtle crashes aligning with defensive programming requirements.

index 377e80f5dd..3a5d0b2bd8 100644

alloc.c +4 -2

@@ -36,19 +36,22 @@ struct alloc_state {
 	int slab_nr, slab_alloc;
 };
 
-struct alloc_state *allocate_alloc_state(void)
+struct alloc_state *alloc_state_alloc(void)
 {
 	return xcalloc(1, sizeof(struct alloc_state));
 }
 
-void clear_alloc_state(struct alloc_state *s)
+void alloc_state_free_and_null(struct alloc_state **s_)
 {
+	struct alloc_state *s = *s_;
+

alloc.h +2 -2

index 3f4a0ad310..87a47a9709 100644
@@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);
 void *alloc_tag_node(struct repository *r);
 void *alloc_object_node(struct repository *r);
 
-struct alloc_state *allocate_alloc_state(void);
-void clear_alloc_state(struct alloc_state *s);
+struct alloc_state *alloc_state_alloc(void);
+void alloc_state_free_and_null(struct alloc_state **s_);
 
 #endif

object.c +3 -5

index c1553ee433..986114a6db 100644
@@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)
 	o->buffer_slab = NULL;
 
 	parsed_object_pool_reset_commit_grafts(o);
-	clear_alloc_state(o->blob_state);
-	clear_alloc_state(o->tree_state);
-	clear_alloc_state(o->commit_state);
-	clear_alloc_state(o->tag_state);
-	clear_alloc_state(o->object_state);
+	alloc_state_free_and_null(&o->blob_state);
+	alloc_state_free_and_null(&o->tree_state);
+	alloc_state_free_and_null(&o->commit_state);

It's probably not worth going back and forth on this too much, but I thought the happy medium was:

if (!s)

	return;

That is, it is perfectly reasonable and friendly for it to be a noop to free-and-null a NULL value (either never initialized, or already freed). The overkill was worrying about whether somebody passed in a NULL double-pointer. I.e., doing:

  alloc_state_free_and_null(&foo);

is reasonable and should be idempotent

It is a programming error, period.  Do not silently return. That's not being defensive.  That is sweeping a problem under the rug.

+    if (!s) return;

Quoted 6 lines · Jeff King

It's probably not worth going back and forth on this too much, but I thought the happy medium was:

   if (!s)
	return;

Quoted 8 lines

That is, it is perfectly reasonable and friendly for it to be a noop to free-and-null a NULL value (either never initialized, or already freed). The overkill was worrying about whether somebody passed in a NULL double-pointer. I.e., doing:

   alloc_state_free_and_null(&foo);

is reasonable and should be idempotent

... when foo == NULL, e.g., after alloc_state_free_and_null(&foo) has just successfully returned?

I can by that argument with the reasoning in the updated log message below. Does it good to everybody?

Thanks.

--- >8 ---
From: ノウラ | Flare <[email protected]>
Subject: [PATCH] alloc: fix dangling pointer in alloc_state cleanup

All callers of clear_alloc_state() immediately free what they cleared, so currently it does not hurt anybody that the alloc_state is left in an unreusable state, but it is an error-prone API. Replace it with a new function that clears but in addition frees the structure, as well as NULLing the pointer that points at it and adjust existing callers.

As it is a moral equivalent of FREE_AND_NULL(), except that what it frees has internal structure that needs to be cleaned, allow the helper to be called twice in a row, by making a call with a pointer to a pointer variable that already is NULLed.

While at it, rename allocate_alloc_state() and name the new function alloc_state_free_and_null(), to follow more closely the function naming convention specified in the CodingGuidelines (namely, functions about S are named with S_ prefix and then verb).

Signed-off-by ノウラ | Flare <[email protected]>

Helped-by Jeff King <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
  alloc.c  | 10 ++++++++--
  alloc.h  |  4 ++--
  object.c | 26 ++++++++++----------------
  3 files changed, 20 insertions(+), 20 deletions(-)

alloc.c +8 -2

index 377e80f5dd..533a045c2a 100644
@@ -36,19 +36,25 @@ struct alloc_state {
  	int slab_nr, slab_alloc;
  };
 
-struct alloc_state *allocate_alloc_state(void)
+struct alloc_state *alloc_state_alloc(void)
  {
  	return xcalloc(1, sizeof(struct alloc_state));
  }
 
-void clear_alloc_state(struct alloc_state *s)
+void alloc_state_free_and_null(struct alloc_state **s_)
  {
+	struct alloc_state *s = *s_;
+
+	if (!s)
+		return;
+
  	while (s->slab_nr > 0) {
  		s->slab_nr--;
  		free(s->slabs[s->slab_nr]);
  	}
 
  	FREE_AND_NULL(s->slabs);
+	FREE_AND_NULL(*s_);
  }
 
  static inline void *alloc_node(struct alloc_state *s, size_t node_size)

alloc.h +2 -2

index 3f4a0ad310..87a47a9709 100644
@@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);
  void *alloc_tag_node(struct repository *r);
  void *alloc_object_node(struct repository *r);
 
-struct alloc_state *allocate_alloc_state(void);
-void clear_alloc_state(struct alloc_state *s);
+struct alloc_state *alloc_state_alloc(void);
+void alloc_state_free_and_null(struct alloc_state **s_);
 
  #endif

object.c +10 -16

index c1553ee433..986114a6db 100644
@@ -517,12 +517,11 @@ struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)
  	memset(o, 0, sizeof(*o));
 
  	o->repo = repo;
-	o->blob_state = allocate_alloc_state();
-	o->tree_state = allocate_alloc_state();
-	o->commit_state = allocate_alloc_state();
-	o->tag_state = allocate_alloc_state();
-	o->object_state = allocate_alloc_state();
-
+	o->blob_state = alloc_state_alloc();
+	o->tree_state = alloc_state_alloc();
+	o->commit_state = alloc_state_alloc();
+	o->tag_state = alloc_state_alloc();
+	o->object_state = alloc_state_alloc();
  	o->is_shallow = -1;
  	CALLOC_ARRAY(o->shallow_stat, 1);
 

@@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)
  	o->buffer_slab = NULL;
 
  	parsed_object_pool_reset_commit_grafts(o);
-	clear_alloc_state(o->blob_state);
-	clear_alloc_state(o->tree_state);
-	clear_alloc_state(o->commit_state);
-	clear_alloc_state(o->tag_state);
-	clear_alloc_state(o->object_state);
+	alloc_state_free_and_null(&o->blob_state);
+	alloc_state_free_and_null(&o->tree_state);
+	alloc_state_free_and_null(&o->commit_state);
+	alloc_state_free_and_null(&o->tag_state);
+	alloc_state_free_and_null(&o->object_state);
  	stat_validity_clear(o->shallow_stat);
-	FREE_AND_NULL(o->blob_state);
-	FREE_AND_NULL(o->tree_state);
-	FREE_AND_NULL(o->commit_state);
-	FREE_AND_NULL(o->tag_state);
-	FREE_AND_NULL(o->object_state);
  	FREE_AND_NULL(o->shallow_stat);
  }

Quoted 1 line

+    if (!s) return;

However, I agree with Peff. After calling alloc_state_free_and_null(&foo) Having foo == NULL is an expected behavior, especially since the function Is designed to free the memory and null out the caller’s pointer using A double pointer ensuring the helper is idempotent

So, calling it again on the same pointer is safe because it simply no-ops if the memory is already freed

Regarding the sanity check, it should be:

+ if (!*s) return;

The reasoning is the following:

=> s is the double pointer (the address of the caller’s pointer) => *s is the actual pointer to the memory we want to free

Thus, we check !*s to allow the function to safely handle already-NULL pointers
But if we instead checked !s then we would be testing whether the caller passed
A NULL double pointer which is a programmer error
So silently returning on !s would hide a bug

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

I agree that is the right thing. But it is equivalent to:

if (!s) return;

since we'll have just assigned "s". Which one to choose is purely a matter of style. Using "*s_" perhaps makes it more clear that we are sanity-checking the input (and could happen even before we assign "s"). Using "s" is consistent with the rest of the function in working with the more direct pointer value. I am happy with either.

It is a programming error, period.  Do not silently return. That's not being defensive.  That is sweeping a problem under the rug.

+    if (!s) return;

Quoted 6 lines · Jeff King

It's probably not worth going back and forth on this too much, but I thought the happy medium was:

   if (!s)
	return;

Quoted 8 lines

That is, it is perfectly reasonable and friendly for it to be a noop to free-and-null a NULL value (either never initialized, or already freed). The overkill was worrying about whether somebody passed in a NULL double-pointer. I.e., doing:

   alloc_state_free_and_null(&foo);

is reasonable and should be idempotent

... when foo == NULL, e.g., after alloc_state_free_and_null(&foo) has just successfully returned?

I can by that argument with the reasoning in the updated log message below. Does it good to everybody?

Thanks.

--- >8 ---
From: ノウラ | Flare <[email protected]>
Subject: [PATCH] alloc: fix dangling pointer in alloc_state cleanup

All callers of clear_alloc_state() immediately free what they cleared, so currently it does not hurt anybody that the alloc_state is left in an unreusable state, but it is an error-prone API. Replace it with a new function that clears but in addition frees the structure, as well as NULLing the pointer that points at it and adjust existing callers.

As it is a moral equivalent of FREE_AND_NULL(), except that what it frees has internal structure that needs to be cleaned, allow the helper to be called twice in a row, by making a call with a pointer to a pointer variable that already is NULLed.

While at it, rename allocate_alloc_state() and name the new function alloc_state_free_and_null(), to follow more closely the function naming convention specified in the CodingGuidelines (namely, functions about S are named with S_ prefix and then verb).

Signed-off-by ノウラ | Flare <[email protected]>

Helped-by Jeff King <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
  alloc.c  | 10 ++++++++--
  alloc.h  |  4 ++--
  object.c | 26 ++++++++++----------------
  3 files changed, 20 insertions(+), 20 deletions(-)

alloc.c +8 -2

index 377e80f5dd..533a045c2a 100644
@@ -36,19 +36,25 @@ struct alloc_state {
  	int slab_nr, slab_alloc;
  };
 
-struct alloc_state *allocate_alloc_state(void)
+struct alloc_state *alloc_state_alloc(void)
  {
  	return xcalloc(1, sizeof(struct alloc_state));
  }
 
-void clear_alloc_state(struct alloc_state *s)
+void alloc_state_free_and_null(struct alloc_state **s_)
  {
+	struct alloc_state *s = *s_;
+
+	if (!s)
+		return;
+
  	while (s->slab_nr > 0) {
  		s->slab_nr--;
  		free(s->slabs[s->slab_nr]);
  	}
 
  	FREE_AND_NULL(s->slabs);
+	FREE_AND_NULL(*s_);
  }
 
  static inline void *alloc_node(struct alloc_state *s, size_t node_size)

alloc.h +2 -2

index 3f4a0ad310..87a47a9709 100644
@@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);
  void *alloc_tag_node(struct repository *r);
  void *alloc_object_node(struct repository *r);
 
-struct alloc_state *allocate_alloc_state(void);
-void clear_alloc_state(struct alloc_state *s);
+struct alloc_state *alloc_state_alloc(void);
+void alloc_state_free_and_null(struct alloc_state **s_);
 
  #endif

object.c +10 -16

index c1553ee433..986114a6db 100644
@@ -517,12 +517,11 @@ struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)
  	memset(o, 0, sizeof(*o));
 
  	o->repo = repo;
-	o->blob_state = allocate_alloc_state();
-	o->tree_state = allocate_alloc_state();
-	o->commit_state = allocate_alloc_state();
-	o->tag_state = allocate_alloc_state();
-	o->object_state = allocate_alloc_state();
-
+	o->blob_state = alloc_state_alloc();
+	o->tree_state = alloc_state_alloc();
+	o->commit_state = alloc_state_alloc();
+	o->tag_state = alloc_state_alloc();
+	o->object_state = alloc_state_alloc();
  	o->is_shallow = -1;
  	CALLOC_ARRAY(o->shallow_stat, 1);
 

@@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)
  	o->buffer_slab = NULL;
 
  	parsed_object_pool_reset_commit_grafts(o);
-	clear_alloc_state(o->blob_state);
-	clear_alloc_state(o->tree_state);
-	clear_alloc_state(o->commit_state);
-	clear_alloc_state(o->tag_state);
-	clear_alloc_state(o->object_state);
+	alloc_state_free_and_null(&o->blob_state);
+	alloc_state_free_and_null(&o->tree_state);
+	alloc_state_free_and_null(&o->commit_state);
+	alloc_state_free_and_null(&o->tag_state);
+	alloc_state_free_and_null(&o->object_state);
  	stat_validity_clear(o->shallow_stat);
-	FREE_AND_NULL(o->blob_state);
-	FREE_AND_NULL(o->tree_state);
-	FREE_AND_NULL(o->commit_state);
-	FREE_AND_NULL(o->tag_state);
-	FREE_AND_NULL(o->object_state);
  	FREE_AND_NULL(o->shallow_stat);
  }

No. I am confused here.

Quoted 2 lines

It is a programming error, period.  Do not silently return. That's not being defensive.  That is sweeping a problem under the rug.

Yet

Quoted 1 line

+    if (!s) return;

However, I agree with Peff. After calling alloc_state_free_and_null(&foo) Having foo == NULL is an expected behavior, especially since the function Is designed to free the memory and null out the caller’s pointer using A double pointer ensuring the helper is idempotent

So, calling it again on the same pointer is safe because it simply no-ops if the memory is already freed

Regarding the sanity check, it should be:

+ if (!*s) return;

The reasoning is the following:

=> s is the double pointer (the address of the caller’s pointer) => *s is the actual pointer to the memory we want to free

Thus, we check !*s to allow the function to safely handle already-NULL pointers But if we instead checked !s then we would be testing whether the caller passed A NULL double pointer which is a programmer error
So silently returning on !s would hide a bug

Finally, I'd appreciate more explicit instructions.

Quoted 137 lines · Junio C Hamano

Quoted 6 lines · Jeff King

It's probably not worth going back and forth on this too much, but I thought the happy medium was:

   if (!s)
    return;

Quoted 8 lines

That is, it is perfectly reasonable and friendly for it to be a noop to free-and-null a NULL value (either never initialized, or already freed). The overkill was worrying about whether somebody passed in a NULL double-pointer. I.e., doing:

   alloc_state_free_and_null(&foo);

is reasonable and should be idempotent

... when foo == NULL, e.g., after alloc_state_free_and_null(&foo) has just successfully returned?

I can by that argument with the reasoning in the updated log message below.  Does it good to everybody?

Thanks.

--- >8 ---
From: ノウラ | Flare <[email protected]>
Subject: [PATCH] alloc: fix dangling pointer in alloc_state cleanup

All callers of clear_alloc_state() immediately free what they cleared, so currently it does not hurt anybody that the alloc_state is left in an unreusable state, but it is an error-prone API. Replace it with a new function that clears but in addition frees the structure, as well as NULLing the pointer that points at it and adjust existing callers.

As it is a moral equivalent of FREE_AND_NULL(), except that what it frees has internal structure that needs to be cleaned, allow the helper to be called twice in a row, by making a call with a pointer to a pointer variable that already is NULLed.

While at it, rename allocate_alloc_state() and name the new function alloc_state_free_and_null(), to follow more closely the function naming convention specified in the CodingGuidelines (namely, functions about S are named with S_ prefix and then verb).

Signed-off-by ノウラ | Flare <[email protected]>

Helped-by Jeff King <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
  alloc.c  | 10 ++++++++--
  alloc.h  |  4 ++--
  object.c | 26 ++++++++++----------------

  3 files changed, 20 insertions(+), 20 deletions(-)

alloc.c +8 -0

index 377e80f5dd..533a045c2a 100644
@@ -36,19 +36,25 @@ struct alloc_state {
       int slab_nr, slab_alloc;
   };
   -struct alloc_state *allocate_alloc_state(void)
+struct alloc_state *alloc_state_alloc(void)
   {
       return xcalloc(1, sizeof(struct alloc_state));
   }
   -void clear_alloc_state(struct alloc_state *s)
+void alloc_state_free_and_null(struct alloc_state **s_)
   {
+    struct alloc_state *s = *s_;
+
+    if (!s)
+        return;
+
       while (s->slab_nr > 0) {
           s->slab_nr--;
           free(s->slabs[s->slab_nr]);
       }
         FREE_AND_NULL(s->slabs);
+    FREE_AND_NULL(*s_);
   }
     static inline void *alloc_node(struct alloc_state *s, size_t node_size)

alloc.h +2 -1

index 3f4a0ad310..87a47a9709 100644
@@ -14,7 +14,7 @@ void *alloc_commit_node(struct repository *r);
   void *alloc_tag_node(struct repository *r);
   void *alloc_object_node(struct repository *r);
   -struct alloc_state *allocate_alloc_state(void);
-void clear_alloc_state(struct alloc_state *s);
+struct alloc_state *alloc_state_alloc(void);
+void alloc_state_free_and_null(struct alloc_state **s_);
     #endif

object.c +5 -6

index c1553ee433..986114a6db 100644
@@ -517,12 +517,11 @@ struct parsed_object_pool 
 *parsed_object_pool_new(struct repository *repo)
       memset(o, 0, sizeof(*o));
         o->repo = repo;
-    o->blob_state = allocate_alloc_state();
-    o->tree_state = allocate_alloc_state();
-    o->commit_state = allocate_alloc_state();
-    o->tag_state = allocate_alloc_state();
-    o->object_state = allocate_alloc_state();
-
+    o->blob_state = alloc_state_alloc();
+    o->tree_state = alloc_state_alloc();
+    o->commit_state = alloc_state_alloc();
+    o->tag_state = alloc_state_alloc();
+    o->object_state = alloc_state_alloc();
       o->is_shallow = -1;
       CALLOC_ARRAY(o->shallow_stat, 1);
   @@ -573,16 +572,11 @@ void parsed_object_pool_clear(struct parsed_object_pool *o)

      o->buffer_slab = NULL; parsed_object_pool_reset_commit_grafts(o); -    clear_alloc_state(o->blob_state);
-    clear_alloc_state(o->tree_state);
-    clear_alloc_state(o->commit_state);
-    clear_alloc_state(o->tag_state);
-    clear_alloc_state(o->object_state);
+    alloc_state_free_and_null(&o->blob_state); +    alloc_state_free_and_null(&o->tree_state); +    alloc_state_free_and_null(&o->commit_state); +    alloc_state_free_and_null(&o->tag_state); +    alloc_state_free_and_null(&o->object_state); stat_validity_clear(o->shallow_stat); -    FREE_AND_NULL(o->blob_state);
-    FREE_AND_NULL(o->tree_state);
-    FREE_AND_NULL(o->commit_state);
-    FREE_AND_NULL(o->tag_state);
-    FREE_AND_NULL(o->object_state); FREE_AND_NULL(o->shallow_stat);
  }

=> s is the double pointer (the address of the caller’s pointer)

By *s I am referring to *s_ so a sanity check with: if (!*s_) return;

Quoted 1 line · ノウラ | Flare

By *s I am referring to *s_ so a sanity check with: if (!*s_) return;

Because we

	s = *s_;

upfront, exactly because we do want the code to segfault if the caller passes NULL to the function (so s_ that is NULL will cause a NULL dereference right there), after that happens checking the NULL ness of s and *s_ is equivalent.

And the whole point of doing "s = *s_" upfront is because readers can easily get confused when they have to deal with double pointers. The only reason why we pass the address of the pointer variable is so that we can assign NULL to it at the very end, and before we can do so, we want to be able inspect the innards of alloc_state object. By dereferencing s_ early into s, the code can work with the object itself without having to worry about following double pointer, so even though if (!*s_) and if (!s) may be equivalent, writing the latter is more in line with the whole reason why we have a variable 's' that is separate from 's_'.

Quoted 11 lines · Jeff King

It's probably not worth going back and forth on this too much, but I thought the happy medium was:

if (!s)

	return;

That is, it is perfectly reasonable and friendly for it to be a noop to free-and-null a NULL value (either never initialized, or already freed). The overkill was worrying about whether somebody passed in a NULL double-pointer. I.e., doing:

Quoted 3 lines

  alloc_state_free_and_null(&foo);

is reasonable and should be idempotent

... when foo == NULL, e.g., after alloc_state_free_and_null(&foo) has just successfully returned?

I can by that argument with the reasoning in the updated log message below. Does it good to everybody?

From: =?UTF-8?q?=E3=83=8E=E3=82=A6=E3=83=A9=20=7C=20Flare?=
<[email protected]>

All callers of clear_alloc_state() immediately free what they cleared, so currently it does not hurt anybody that the alloc_state is left in an unreusable state, but it is an error-prone API. Replace it with a new function that clears but in addition frees the structure, as well as NULLing the pointer that points at it and adjust existing callers.

As it is a moral equivalent of FREE_AND_NULL(), except that what it frees has internal structure that needs to be cleaned, allow the helper to be called twice in a row, by making a call with a pointer to a pointer variable that already is NULLed.

While at it, rename allocate_alloc_state() and name the new function alloc_state_free_and_null(), to follow more closely the function naming convention specified in the CodingGuidelines (namely, functions about S are named with S_ prefix and then verb).

Signed-off-by ノウラ | Flare <[email protected]>

Helped-by Jeff King <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
alloc: fix dangling pointer in alloc_state cleanup

cc Torsten Bögershausen [email protected] cc: Jeff King [email protected]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2040%2Fnouraellm%2Ffix-dangling-pointer-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2040/nouraellm/fix-dangling-pointer-v6
Pull-Request: https://github.com/git/git/pull/2040