[PATCH] fix: possible memory leak in unxz()

[PATCH] fix: possible memory leak in unxz()

[Vishnu Sanal T]

Fixes possible memory leak in the function unxz() in
lib/decompress_unxz.c forgets to free the pointer 'in', when
the statement if (fill == NULL && flush == NULL) is true.

Signed-off-by: Vishnu Sanal T <t.v.s10123@gmail.com>
---
 lib/decompress_unxz.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/decompress_unxz.c b/lib/decompress_unxz.c
index 32138bb8ef77..8d58207ca1db 100644
--- a/lib/decompress_unxz.c
+++ b/lib/decompress_unxz.c
@@ -343,13 +343,13 @@ STATIC int INIT unxz(unsigned char *in, long in_size,
 			}
 		} while (ret == XZ_OK);
 
-		if (must_free_in)
-			free(in);
-
 		if (flush != NULL)
 			free(b.out);
 	}
 
+	if (must_free_in)
+		free(in);
+
 	if (in_used != NULL)
 		*in_used += b.in_pos;
 
-- 
2.46.2

Re: [PATCH] fix: possible memory leak in unxz()

[Lasse Collin]

On 2024-10-06 Vishnu Sanal T wrote:
> Fixes possible memory leak in the function unxz() in
> lib/decompress_unxz.c forgets to free the pointer 'in', when
> the statement if (fill == NULL && flush == NULL) is true.

unxz() looks confusing but a memory leak shouldn't be possible. If "in"
is NULL then "fill" must be non-NULL. Otherwise the caller isn't
following the API defined in include/linux/decompress/generic.h and
things might break in other ways too. (I find the generic.h API
somewhat hairy.)

Note that both "in" and "fill" are allowed to be non-NULL at the same
time. That's why the code checks for "in == NULL" instead of "fill !=
NULL" before allocating memory.

The current malloc+free usage in unxz() is such that freeing is done in
reverse order compared to mallocs. I guess it's not important. At least
include/linux/decompress/mm.h doesn't care.

I think your patch makes it more obvious that there is no memory leak.
Perhaps it might help static analyzers too. The change shouldn't break
anything. On the other hand, it makes it less obvious to human readers
that free(in) is only needed when fill != NULL.

I didn't test but I guess that it shouldn't increase code size when
fill == NULL && flush == NULL. That is, compilers should see that
must_free_in must be false in that case. Even if they don't, it's not
many bytes.

I'm not in favor of this patch because the unxz() function is quite
delicate due to the API it is implementing, and the patch doesn't fix a
memory leak (unless the API is misused and then those misuses should be
fixed). On the other hand, I'm not against this patch either *if* other
developers think this is an improvement. In that case the commit
message should be changed since it's not fixing an actual memory leak.

Thanks!

-- 
Lasse Collin

Re: [PATCH] fix: possible memory leak in unxz()

[Andrew Morton]

On Sun,  6 Oct 2024 12:55:43 +0530 Vishnu Sanal T <t.v.s10123@gmail.com> wrote:

> Fixes possible memory leak in the function unxz() in
> lib/decompress_unxz.c forgets to free the pointer 'in', when
> the statement if (fill == NULL && flush == NULL) is true.
> 
> ...
>
> --- a/lib/decompress_unxz.c
> +++ b/lib/decompress_unxz.c
> @@ -343,13 +343,13 @@ STATIC int INIT unxz(unsigned char *in, long in_size,
>  			}
>  		} while (ret == XZ_OK);
>  
> -		if (must_free_in)
> -			free(in);
> -
>  		if (flush != NULL)
>  			free(b.out);
>  	}
>  
> +	if (must_free_in)
> +		free(in);
> +
>  	if (in_used != NULL)
>  		*in_used += b.in_pos;
>  

Looks correct.

must_free_in needn't exist - `in' is always non-NULL here.  And
free(NULL) is OK anwyay.

Re: [PATCH] fix: possible memory leak in unxz()

[Lasse Collin]

In short, I dont want this patch to be merged, especially with its
current misleading commit message.

On 2024-10-07 Andrew Morton wrote:
> must_free_in needn't exist - `in' is always non-NULL here.

No, must_free_in is needed. The API (defined in
include/linux/decompress/generic.h) packs three different functions
into a single function:

1. Buffer to buffer (from "in"/"inbuf" to "out"/"outbuf")
2. Buffer ("in"/"inbuf") to callback function (flush)
3. Callback function (fill) to callback function (flush)

The callback functions need input and output buffers. For "flush", the
buffer always has to be allocated.

But, for some reason, generic.h says that "fill" can use the input
buffer provided by the caller when it's non-NULL. Thus, unxz()
allocates "in" conditionally and must_free_in is needed to avoid
freeing "in" when unxz() didn't allocate it.

I don't know if anything actually uses this subvariant where fill !=
NULL && in != NULL. Several other lib/decompress_*.c files support it
too although unlzo and unlz4 don't. So perhaps the feature is an
unneeded complication. Getting rid of this feature could make the code
less confusing. It would also allow making the "in" pointer
const-correct for buffer-to-{buffer,callback} cases.

> And free(NULL) is OK anwyay.

No, it's not. The free() implementation in
include/linux/decompress/mm.h doesn't check for NULL; each free() just
decrements the allocation counter. Thus free(NULL) would desync it.

As I wrote two days ago[1], this patch doesn't fix a memory leak or any
other bugs, and thus the commit message is incorrect. Still, I wasn't
really against the patch two days ago but I am now. I see the code
being misunderstood, and I feel this patch doesn't make it more
understandable.

[1] https://lore.kernel.org/lkml/20241006211720.5d7199b0@kaneli/

-- 
Lasse Collin