[PATCH] reftable/writer: fix memory leak when `padded_write()` fails

[PATCH] reftable/writer: fix memory leak when `padded_write()` fails

[Lidong Yan via GitGitGadget]

From: Lidong Yan <502024330056@smail.nju.edu.cn>

In reftable/writer.c:padded_write(), if w->writer failed, zeroed
allocated in `reftable_calloc` will leak. w->writer could be
`reftable_write_data` in reftable/stack.c, and could fail due to
some write error. Simply add reftable_free(zeroed) will solve this
problem.

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
    reftable/writer: fix memory leak when padded_write() fails
    
    In reftable/writer.c:padded_write(), if w->writer failed, zeroed
    allocated in reftable_calloc will leak. w->writer could be
    reftable_write_data in reftable/stack.c, and could fail due to some
    write error. Simply add reftable_free(zeroed) will solve this problem.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1958%2Fbrandb97%2Ffix-reftable-padded-write-leak-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1958/brandb97/fix-reftable-padded-write-leak-v1
Pull-Request: https://github.com/git/git/pull/1958

 reftable/writer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/reftable/writer.c b/reftable/writer.c
index cb16f71be49..3ceb3742888 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -57,8 +57,10 @@ static int padded_write(struct reftable_writer *w, uint8_t *data, size_t len,
 			return -1;
 
 		n = w->write(w->write_arg, zeroed, w->pending_padding);
-		if (n < 0)
+		if (n < 0) {
+			reftable_free(zeroed);
 			return n;
+		}
 
 		w->pending_padding = 0;
 		reftable_free(zeroed);

base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75
-- 
gitgitgadget

[PATCH v2] reftable/writer: fix memory leak if write fails

[Lidong Yan via GitGitGadget]

From: Lidong Yan <502024330056@smail.nju.edu.cn>

In reftable/writer.c:padded_write(), if w->writer failed, zeroed
allocated in `reftable_calloc` will leak. w->writer could be
`reftable_write_data` in reftable/stack.c, and could fail due to
some write error. Simply add reftable_free(zeroed) will solve this
problem.

In reftable/writer.c:writer_index_hash(), if `reftable_buf_add` failed,
key allocated by `reftable_malloc` will not be insert into `obj_index_tree`
thus leaks. Simple add reftable_free(key) will solve this problem.

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
    reftable/writer: fix memory leak when padded_write() fails
    
    In reftable/writer.c:padded_write(), if w->writer failed, zeroed
    allocated in reftable_calloc will leak. w->writer could be
    reftable_write_data in reftable/stack.c, and could fail due to some
    write error. Simply add reftable_free(zeroed) will solve this problem.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1958%2Fbrandb97%2Ffix-reftable-padded-write-leak-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1958/brandb97/fix-reftable-padded-write-leak-v2
Pull-Request: https://github.com/git/git/pull/1958

Range-diff vs v1:

 1:  2023d6791ef ! 1:  9329ae2d478 reftable/writer: fix memory leak when `padded_write()` fails
     @@ Metadata
      Author: Lidong Yan <502024330056@smail.nju.edu.cn>
      
       ## Commit message ##
     -    reftable/writer: fix memory leak when `padded_write()` fails
     +    reftable/writer: fix memory leak if write fails
      
          In reftable/writer.c:padded_write(), if w->writer failed, zeroed
          allocated in `reftable_calloc` will leak. w->writer could be
     @@ Commit message
          some write error. Simply add reftable_free(zeroed) will solve this
          problem.
      
     +    In reftable/writer.c:writer_index_hash(), if `reftable_buf_add` failed,
     +    key allocated by `reftable_malloc` will not be insert into `obj_index_tree`
     +    thus leaks. Simple add reftable_free(key) will solve this problem.
     +
          Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
      
       ## reftable/writer.c ##
     @@ reftable/writer.c: static int padded_write(struct reftable_writer *w, uint8_t *d
       
       		w->pending_padding = 0;
       		reftable_free(zeroed);
     +@@ reftable/writer.c: static int writer_index_hash(struct reftable_writer *w, struct reftable_buf *has
     + 
     + 		reftable_buf_reset(&key->hash);
     + 		err = reftable_buf_add(&key->hash, hash->buf, hash->len);
     +-		if (err < 0)
     ++		if (err < 0) {
     ++			reftable_free(key);
     + 			return err;
     ++		}
     + 		tree_insert(&w->obj_index_tree, key,
     + 			    &obj_index_tree_node_compare);
     + 	} else {


 reftable/writer.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/reftable/writer.c b/reftable/writer.c
index cb16f71be49..3b4ebdd6dce 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -57,8 +57,10 @@ static int padded_write(struct reftable_writer *w, uint8_t *data, size_t len,
 			return -1;
 
 		n = w->write(w->write_arg, zeroed, w->pending_padding);
-		if (n < 0)
+		if (n < 0) {
+			reftable_free(zeroed);
 			return n;
+		}
 
 		w->pending_padding = 0;
 		reftable_free(zeroed);
@@ -256,8 +258,10 @@ static int writer_index_hash(struct reftable_writer *w, struct reftable_buf *has
 
 		reftable_buf_reset(&key->hash);
 		err = reftable_buf_add(&key->hash, hash->buf, hash->len);
-		if (err < 0)
+		if (err < 0) {
+			reftable_free(key);
 			return err;
+		}
 		tree_insert(&w->obj_index_tree, key,
 			    &obj_index_tree_node_compare);
 	} else {

base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75
-- 
gitgitgadget

Re: [PATCH v2] reftable/writer: fix memory leak if write fails

[Patrick Steinhardt]

On Sun, May 11, 2025 at 04:16:04PM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <502024330056@smail.nju.edu.cn>
> 
> In reftable/writer.c:padded_write(), if w->writer failed, zeroed
> allocated in `reftable_calloc` will leak. w->writer could be
> `reftable_write_data` in reftable/stack.c, and could fail due to
> some write error. Simply add reftable_free(zeroed) will solve this
> problem.
> 
> In reftable/writer.c:writer_index_hash(), if `reftable_buf_add` failed,
> key allocated by `reftable_malloc` will not be insert into `obj_index_tree`
> thus leaks. Simple add reftable_free(key) will solve this problem.

Nit: I think it would be sensible to split these up into two commits, as
they touch different areas of the code.

> diff --git a/reftable/writer.c b/reftable/writer.c
> index cb16f71be49..3b4ebdd6dce 100644
> --- a/reftable/writer.c
> +++ b/reftable/writer.c
> @@ -57,8 +57,10 @@ static int padded_write(struct reftable_writer *w, uint8_t *data, size_t len,
>  			return -1;
>  
>  		n = w->write(w->write_arg, zeroed, w->pending_padding);
> -		if (n < 0)
> +		if (n < 0) {
> +			reftable_free(zeroed);
>  			return n;
> +		}
>  
>  		w->pending_padding = 0;
>  		reftable_free(zeroed);

Makes sense.

> @@ -256,8 +258,10 @@ static int writer_index_hash(struct reftable_writer *w, struct reftable_buf *has
>  
>  		reftable_buf_reset(&key->hash);
>  		err = reftable_buf_add(&key->hash, hash->buf, hash->len);
> -		if (err < 0)
> +		if (err < 0) {
> +			reftable_free(key);
>  			return err;
> +		}
>  		tree_insert(&w->obj_index_tree, key,
>  			    &obj_index_tree_node_compare);
>  	} else {

Makes sense, as well. We want to add the node to the object index tree,
but if a step before fails then we naturally want to free the node.

Patrick

Re: [PATCH v2] reftable/writer: fix memory leak if write fails

[lidongyan]

On 12/5/2025 at 16:25，Patrick Steinhardt <ps@pks.im> wrote：
> Nit: I think it would be sensible to split these up into two commits, as
> they touch different areas of the code.
I will split them in the next patch.

[PATCH v3 0/2] reftable/writer: fix memory leak when write fails

[Lidong Yan via GitGitGadget]

Lidong Yan (2):
  reftable/writer: fix memory leak when `padded_write()` fails
  reftable/writer: fix memory leak when `writer_index_hash()` fails

 reftable/writer.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)


base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1958%2Fbrandb97%2Ffix-reftable-padded-write-leak-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1958/brandb97/fix-reftable-padded-write-leak-v3
Pull-Request: https://github.com/git/git/pull/1958

Range-diff vs v2:

 1:  9329ae2d478 ! 1:  13ebdd672ff reftable/writer: fix memory leak if write fails
     @@ Metadata
      Author: Lidong Yan <502024330056@smail.nju.edu.cn>
      
       ## Commit message ##
     -    reftable/writer: fix memory leak if write fails
     +    reftable/writer: fix memory leak when `padded_write()` fails
      
          In reftable/writer.c:padded_write(), if w->writer failed, zeroed
          allocated in `reftable_calloc` will leak. w->writer could be
     @@ Commit message
          some write error. Simply add reftable_free(zeroed) will solve this
          problem.
      
     -    In reftable/writer.c:writer_index_hash(), if `reftable_buf_add` failed,
     -    key allocated by `reftable_malloc` will not be insert into `obj_index_tree`
     -    thus leaks. Simple add reftable_free(key) will solve this problem.
     -
          Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
      
       ## reftable/writer.c ##
     @@ reftable/writer.c: static int padded_write(struct reftable_writer *w, uint8_t *d
       
       		w->pending_padding = 0;
       		reftable_free(zeroed);
     -@@ reftable/writer.c: static int writer_index_hash(struct reftable_writer *w, struct reftable_buf *has
     - 
     - 		reftable_buf_reset(&key->hash);
     - 		err = reftable_buf_add(&key->hash, hash->buf, hash->len);
     --		if (err < 0)
     -+		if (err < 0) {
     -+			reftable_free(key);
     - 			return err;
     -+		}
     - 		tree_insert(&w->obj_index_tree, key,
     - 			    &obj_index_tree_node_compare);
     - 	} else {
 -:  ----------- > 2:  64f778ce2ba reftable/writer: fix memory leak when `writer_index_hash()` fails

-- 
gitgitgadget

[PATCH v3 1/2] reftable/writer: fix memory leak when `padded_write()` fails

[Lidong Yan via GitGitGadget]

From: Lidong Yan <502024330056@smail.nju.edu.cn>

In reftable/writer.c:padded_write(), if w->writer failed, zeroed
allocated in `reftable_calloc` will leak. w->writer could be
`reftable_write_data` in reftable/stack.c, and could fail due to
some write error. Simply add reftable_free(zeroed) will solve this
problem.

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
 reftable/writer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/reftable/writer.c b/reftable/writer.c
index cb16f71be49e..3ceb37428887 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -57,8 +57,10 @@ static int padded_write(struct reftable_writer *w, uint8_t *data, size_t len,
 			return -1;
 
 		n = w->write(w->write_arg, zeroed, w->pending_padding);
-		if (n < 0)
+		if (n < 0) {
+			reftable_free(zeroed);
 			return n;
+		}
 
 		w->pending_padding = 0;
 		reftable_free(zeroed);
-- 
gitgitgadget

[PATCH v3 2/2] reftable/writer: fix memory leak when `writer_index_hash()` fails

[Lidong Yan via GitGitGadget]

From: Lidong Yan <502024330056@smail.nju.edu.cn>

In reftable/writer.c:writer_index_hash(), if `reftable_buf_add` failed,
key allocated by `reftable_malloc` will not be insert into `obj_index_tree`
thus leaks. Simple add reftable_free(key) will solve this problem.

Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
---
 reftable/writer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/reftable/writer.c b/reftable/writer.c
index 3ceb37428887..3b4ebdd6dced 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -258,8 +258,10 @@ static int writer_index_hash(struct reftable_writer *w, struct reftable_buf *has
 
 		reftable_buf_reset(&key->hash);
 		err = reftable_buf_add(&key->hash, hash->buf, hash->len);
-		if (err < 0)
+		if (err < 0) {
+			reftable_free(key);
 			return err;
+		}
 		tree_insert(&w->obj_index_tree, key,
 			    &obj_index_tree_node_compare);
 	} else {
-- 
gitgitgadget

Re: [PATCH v3 0/2] reftable/writer: fix memory leak when write fails

[Patrick Steinhardt]

On Mon, May 12, 2025 at 12:49:02PM +0000, Lidong Yan via GitGitGadget wrote:
> Lidong Yan (2):
>   reftable/writer: fix memory leak when `padded_write()` fails
>   reftable/writer: fix memory leak when `writer_index_hash()` fails
> 
>  reftable/writer.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Thanks, these look good to me.

Patrick