threads / patch / 61670

patchobject-file: fix leak on conversion failure

Subject: [PATCH] object-file: fix leak on conversion failure

## tl;dr

3 messages between Jun 22, 2024 and Jun 24, 2024. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Eric Wong· Jun 22, 2024, 04:36 UTC · lore

I'm not sure exactly how to trigger the leak, but it seems fairly obvious that the `content' buffer should be freed even if convert_object_file() fails. Noticed while working in this area on unrelated things.

Signed-off-by: Eric Wong <e@80x24.org>
---
 object-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to object-file.c +1 −1

diff --git a/object-file.c b/object-file.c
index d3cf4b8b2e..00c8f1039b 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1711,9 +1711,9 @@ static int oid_object_info_convert(struct repository *r,
 			ret = convert_object_file(&outbuf,
 						  the_hash_algo, input_algo,
 						  content, size, type, !do_die);
+			free(content);
 			if (ret == -1)
 				return -1;
-			free(content);
 			size = outbuf.len;
 			content = strbuf_detach(&outbuf, NULL);
 		}

Derrick Stolee· Jun 23, 2024, 16:34 UTC · re: Eric Wong · lore

Re: [PATCH] object-file: fix leak on conversion failure

On 6/22/24 12:36 AM, Eric Wong wrote:

> I'm not sure exactly how to trigger the leak, but it seems fairly
> obvious that the `content' buffer should be freed even if
> convert_object_file() fails.  Noticed while working in this area
> on unrelated things.

Definitely a good thing to include, even if it is unlikely to be hit frequently in common scenarios.

Show 7 quoted lines

>   			ret = convert_object_file(&outbuf,
>   						  the_hash_algo, input_algo,
>   						  content, size, type, !do_die);
> +			free(content);
>   			if (ret == -1)
>   				return -1;
> -			free(content);

I looked at the context of this function to see that 'content' was local to the method, so was not "owned" by something outside of the method that might expect to reuse the buffer on failure.

Thanks, -Stolee

Junio C Hamano· Jun 24, 2024, 16:08 UTC · re: Derrick Stolee · lore

Re: [PATCH] object-file: fix leak on conversion failure

Derrick Stolee <stolee@gmail.com> writes:

Show 20 quoted lines

> On 6/22/24 12:36 AM, Eric Wong wrote:
>> I'm not sure exactly how to trigger the leak, but it seems fairly
>> obvious that the `content' buffer should be freed even if
>> convert_object_file() fails.  Noticed while working in this area
>> on unrelated things.
>
> Definitely a good thing to include, even if it is unlikely to
> be hit frequently in common scenarios.
>
>>   			ret = convert_object_file(&outbuf,
>>   						  the_hash_algo, input_algo,
>>   						  content, size, type, !do_die);
>> +			free(content);
>>   			if (ret == -1)
>>   				return -1;
>> -			free(content);
>
> I looked at the context of this function to see that 'content'
> was local to the method, so was not "owned" by something outside
> of the method that might expect to reuse the buffer on failure.

Thanks, both.

Will queue.