[PATCH] SquashFS, XZ: Don't use uninitialized variable in squashfs_xz_uncompress

[PATCH] SquashFS, XZ: Don't use uninitialized variable in squashfs_xz_uncompress

[Jesper Juhl]

In fs/squashfs/xz_wrapper.c::squashfs_xz_uncompress() we have this code:

	enum xz_ret xz_err;
	...
	do {
		if (stream->buf.in_pos == stream->buf.in_size && k < b) {
		... [nothing that assigns to 'xz_err'] ...
			if (avail == 0) {
				offset = 0;
				put_bh(bh[k++]);
				continue;
			}
		...
	} while (xz_err == XZ_OK);

If we hit that 'continue' statement the first time through, then the 
'while' condition will be testing an uninitialized 'xz_err' - not what we 
want.

This patch should take care of the problem by making sure that 'xz_err' is 
initialized to 'XZ_OK' at the start of the function.

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 xz_wrapper.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

  compile tested only.

diff --git a/fs/squashfs/xz_wrapper.c b/fs/squashfs/xz_wrapper.c
index 856756c..daf76c3 100644
--- a/fs/squashfs/xz_wrapper.c
+++ b/fs/squashfs/xz_wrapper.c
@@ -74,7 +74,7 @@ static int squashfs_xz_uncompress(struct squashfs_sb_info *msblk, void **buffer,
 	struct buffer_head **bh, int b, int offset, int length, int srclength,
 	int pages)
 {
-	enum xz_ret xz_err;
+	enum xz_ret xz_err = XZ_OK;
 	int avail, total = 0, k = 0, page = 0;
 	struct squashfs_xz *stream = msblk->stream;
 


-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] SquashFS, XZ: Don't use uninitialized variable in squashfs_xz_uncompress

[Andrew Morton]

On Thu, 20 Jan 2011 21:53:23 +0100 (CET)
Jesper Juhl <jj@chaosbits.net> wrote:

> In fs/squashfs/xz_wrapper.c::squashfs_xz_uncompress() we have this code:
> 
> 	enum xz_ret xz_err;
> 	...
> 	do {
> 		if (stream->buf.in_pos == stream->buf.in_size && k < b) {
> 		... [nothing that assigns to 'xz_err'] ...
> 			if (avail == 0) {
> 				offset = 0;
> 				put_bh(bh[k++]);
> 				continue;
> 			}
> 		...
> 	} while (xz_err == XZ_OK);
> 
> If we hit that 'continue' statement the first time through, then the 
> 'while' condition will be testing an uninitialized 'xz_err' - not what we 
> want.
> 
> This patch should take care of the problem by making sure that 'xz_err' is 
> initialized to 'XZ_OK' at the start of the function.
> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> ---
>  xz_wrapper.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>   compile tested only.
> 
> diff --git a/fs/squashfs/xz_wrapper.c b/fs/squashfs/xz_wrapper.c
> index 856756c..daf76c3 100644
> --- a/fs/squashfs/xz_wrapper.c
> +++ b/fs/squashfs/xz_wrapper.c
> @@ -74,7 +74,7 @@ static int squashfs_xz_uncompress(struct squashfs_sb_info *msblk, void **buffer,
>  	struct buffer_head **bh, int b, int offset, int length, int srclength,
>  	int pages)
>  {
> -	enum xz_ret xz_err;
> +	enum xz_ret xz_err = XZ_OK;
>  	int avail, total = 0, k = 0, page = 0;
>  	struct squashfs_xz *stream = msblk->stream;
>  

hm, maybe.  The handling of the `avail == 0' case looks odd.  It sits
there in a loop doing wait_on_buffer() against buffers which it will
never use and possible reporting -EIO for a buffer which it didn't use,
which seems bogus.

Phillip, please check?

Re: [PATCH] SquashFS, XZ: Don't use uninitialized variable in squashfs_xz_uncompress

[Phillip Lougher]

On 24/01/11 22:43, Andrew Morton wrote:
> On Thu, 20 Jan 2011 21:53:23 +0100 (CET)
> Jesper Juhl<jj@chaosbits.net>  wrote:
>
>> In fs/squashfs/xz_wrapper.c::squashfs_xz_uncompress() we have this code:
>>
>> 	enum xz_ret xz_err;
>> 	...
>> 	do {
>> 		if (stream->buf.in_pos == stream->buf.in_size&&  k<  b) {
>> 		... [nothing that assigns to 'xz_err'] ...
>> 			if (avail == 0) {
>> 				offset = 0;
>> 				put_bh(bh[k++]);
>> 				continue;
>
> hm, maybe.  The handling of the `avail == 0' case looks odd.  It sits
> there in a loop doing wait_on_buffer() against buffers which it will
> never use and possible reporting -EIO for a buffer which it didn't use,
> which seems bogus.

Hi Andrew,

I'm just about to send out a patch that fixes this uninitialised variable
bug by way of getting rid of that if (avail == 0) case.

The avail == 0 case is a workaround for the fact that the caller code
can occasionally pass a buffer head that has already been
consumed (read offset into buffer head is at the end of the buffer).

Phillip