[PATCH] btrfs-progs: init uninitialized output buf for btrfs-restore

[PATCH] btrfs-progs: init uninitialized output buf for btrfs-restore

[Gui Hecheng]

A memory problem reported by valgrind as follows:
	=== Syscall param pwrite64(buf) points to uninitialised byte(s)
When running:
	# valgrind --leak-check=yes btrfs restore /dev/sda9 /mnt/backup

Because the output buf size is alloced with malloc, but the length of
output data is shorter than the sizeof(buf), so valgrind report
uninitialised byte(s).
We could use calloc to repalce malloc and clear this WARNING away.

Reported-by: Marc Dietrich <marvin24@gmx.de>
Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 cmds-restore.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmds-restore.c b/cmds-restore.c
index cbda6bb..bb72311 100644
--- a/cmds-restore.c
+++ b/cmds-restore.c
@@ -251,7 +251,7 @@ static int copy_one_inline(int fd, struct btrfs_path *path, u64 pos)
 	}
 
 	ram_size = btrfs_file_extent_ram_bytes(leaf, fi);
-	outbuf = malloc(ram_size);
+	outbuf = calloc(1, ram_size);
 	if (!outbuf) {
 		fprintf(stderr, "No memory\n");
 		return -ENOMEM;
@@ -320,7 +320,7 @@ static int copy_one_extent(struct btrfs_root *root, int fd,
 	}
 
 	if (compress != BTRFS_COMPRESS_NONE) {
-		outbuf = malloc(ram_size);
+		outbuf = calloc(1, ram_size);
 		if (!outbuf) {
 			fprintf(stderr, "No memory\n");
 			free(inbuf);
-- 
1.8.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-

Re: [PATCH] btrfs-progs: init uninitialized output buf for btrfs-restore

[Marc Dietrich]

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

Hi Gui,

Am Donnerstag, 21. August 2014, 11:35:36 schrieb Gui Hecheng:
> A memory problem reported by valgrind as follows:
> 	=== Syscall param pwrite64(buf) points to uninitialised byte(s)
> When running:
> 	# valgrind --leak-check=yes btrfs restore /dev/sda9 /mnt/backup
> 
> Because the output buf size is alloced with malloc, but the length of
> output data is shorter than the sizeof(buf), so valgrind report
> uninitialised byte(s).
> We could use calloc to repalce malloc and clear this WARNING away.

yes, the warning vanished. But the reads from free'd memory make me more 
worring...

Marc

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] btrfs-progs: init uninitialized output buf for btrfs-restore

[Gui Hecheng]

On Thu, 2014-08-21 at 10:14 +0200, Marc Dietrich wrote:
> Hi Gui,
> 
> Am Donnerstag, 21. August 2014, 11:35:36 schrieb Gui Hecheng:
> > A memory problem reported by valgrind as follows:
> > 	=== Syscall param pwrite64(buf) points to uninitialised byte(s)
> > When running:
> > 	# valgrind --leak-check=yes btrfs restore /dev/sda9 /mnt/backup
> > 
> > Because the output buf size is alloced with malloc, but the length of
> > output data is shorter than the sizeof(buf), so valgrind report
> > uninitialised byte(s).
> > We could use calloc to repalce malloc and clear this WARNING away.
> 
> yes, the warning vanished. But the reads from free'd memory make me more 
> worring...

Ah, yeah, I am looking into it, hope that I can do some help :)

> Marc

Re: [PATCH] btrfs-progs: init uninitialized output buf for btrfs-restore

[Eric Sandeen]

On 8/20/14, 10:35 PM, Gui Hecheng wrote:
> A memory problem reported by valgrind as follows:
> 	=== Syscall param pwrite64(buf) points to uninitialised byte(s)
> When running:
> 	# valgrind --leak-check=yes btrfs restore /dev/sda9 /mnt/backup
> 
> Because the output buf size is alloced with malloc, but the length of
> output data is shorter than the sizeof(buf), so valgrind report
> uninitialised byte(s).
> We could use calloc to repalce malloc and clear this WARNING away.

It clears the valgrind error away, but does it hide a real bug?

The code does this:

        ram_size = btrfs_file_extent_ram_bytes(leaf, fi);
        outbuf = malloc(ram_size);
        if (!outbuf) {
                fprintf(stderr, "No memory\n");
                return -ENOMEM;
        }

        ret = decompress(buf, outbuf, len, &ram_size, compress);
        if (ret) {
                free(outbuf);
                return ret;
        }

        done = pwrite(fd, outbuf, ram_size, pos);

Now, I don't know the details of the decompression routines, but
it sure *looks* to me like we have found out that "ram size" is the
size of the decompressed data, and so we allocate that much.

If valgrind detects that when we write ram_size bytes, some of
them are uninitialized, doesn't that mean that something has
gone wrong in decompression?

using calloc shuts up the warning, sure, but ...

Marc, are you using zlib or lzo?

If zlib, maybe this in decompress_zlib is a problem:

        (void)inflateEnd(&strm);
        return 0;
}

"inflateEnd returns Z_OK if success, Z_STREAM_ERROR if the stream state was inconsistent."

Josef, any idea why return value is cast away there?

Thanks,
-Eric

> Reported-by: Marc Dietrich <marvin24@gmx.de>
> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> ---
>  cmds-restore.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds-restore.c b/cmds-restore.c
> index cbda6bb..bb72311 100644
> --- a/cmds-restore.c
> +++ b/cmds-restore.c
> @@ -251,7 +251,7 @@ static int copy_one_inline(int fd, struct btrfs_path *path, u64 pos)
>  	}
>  
>  	ram_size = btrfs_file_extent_ram_bytes(leaf, fi);
> -	outbuf = malloc(ram_size);
> +	outbuf = calloc(1, ram_size);
>  	if (!outbuf) {
>  		fprintf(stderr, "No memory\n");
>  		return -ENOMEM;
> @@ -320,7 +320,7 @@ static int copy_one_extent(struct btrfs_root *root, int fd,
>  	}
>  
>  	if (compress != BTRFS_COMPRESS_NONE) {
> -		outbuf = malloc(ram_size);
> +		outbuf = calloc(1, ram_size);
>  		if (!outbuf) {
>  			fprintf(stderr, "No memory\n");
>  			free(inbuf);
> 

Re: [PATCH] btrfs-progs: init uninitialized output buf for btrfs-restore

[Eric Sandeen]

On 8/21/14, 1:42 PM, Eric Sandeen wrote:
> On 8/20/14, 10:35 PM, Gui Hecheng wrote:
>> A memory problem reported by valgrind as follows:
>> 	=== Syscall param pwrite64(buf) points to uninitialised byte(s)
>> When running:
>> 	# valgrind --leak-check=yes btrfs restore /dev/sda9 /mnt/backup
>>
>> Because the output buf size is alloced with malloc, but the length of
>> output data is shorter than the sizeof(buf), so valgrind report
>> uninitialised byte(s).
>> We could use calloc to repalce malloc and clear this WARNING away.
> 
> It clears the valgrind error away, but does it hide a real bug?

Maybe the relevant question for Marc is - did you get decompression
errors during restore?  if so then I guess it all makes sense, and
the proposed patch seems sane after all, sorry.

-Eric

Re: [PATCH] btrfs-progs: init uninitialized output buf for btrfs-restore

[Marc Dietrich]

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

Hi Eric,

Am Donnerstag, 21. August 2014, 13:56:49 schrieb Eric Sandeen:
> On 8/21/14, 1:42 PM, Eric Sandeen wrote:
> > On 8/20/14, 10:35 PM, Gui Hecheng wrote:
> >> A memory problem reported by valgrind as follows:
> >> 	=== Syscall param pwrite64(buf) points to uninitialised byte(s)
> >> 
> >> When running:
> >> 	# valgrind --leak-check=yes btrfs restore /dev/sda9 /mnt/backup
> >> 
> >> Because the output buf size is alloced with malloc, but the length of
> >> output data is shorter than the sizeof(buf), so valgrind report
> >> uninitialised byte(s).
> >> We could use calloc to repalce malloc and clear this WARNING away.
> > 
> > It clears the valgrind error away, but does it hide a real bug?
> 
> Maybe the relevant question for Marc is - did you get decompression
> errors during restore?  if so then I guess it all makes sense, and
> the proposed patch seems sane after all, sorry.

I use lzo and yes, I got decompression errors.

Marc

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] btrfs-progs: init uninitialized output buf for btrfs-restore

[Eric Sandeen]

On 8/22/14, 2:35 AM, Marc Dietrich wrote:
> Hi Eric,
> 
> Am Donnerstag, 21. August 2014, 13:56:49 schrieb Eric Sandeen:
>> On 8/21/14, 1:42 PM, Eric Sandeen wrote:
>>> On 8/20/14, 10:35 PM, Gui Hecheng wrote:
>>>> A memory problem reported by valgrind as follows:
>>>> 	=== Syscall param pwrite64(buf) points to uninitialised byte(s)
>>>>
>>>> When running:
>>>> 	# valgrind --leak-check=yes btrfs restore /dev/sda9 /mnt/backup
>>>>
>>>> Because the output buf size is alloced with malloc, but the length of
>>>> output data is shorter than the sizeof(buf), so valgrind report
>>>> uninitialised byte(s).
>>>> We could use calloc to repalce malloc and clear this WARNING away.
>>>
>>> It clears the valgrind error away, but does it hide a real bug?
>>
>> Maybe the relevant question for Marc is - did you get decompression
>> errors during restore?  if so then I guess it all makes sense, and
>> the proposed patch seems sane after all, sorry.
> 
> I use lzo and yes, I got decompression errors.

Ok, ignore me then, I'm sorry - it all makes sense, Gui's patch
included.  I didn't have my head on straight.

Thanks,
-Eric

Re: [PATCH] btrfs-progs: init uninitialized output buf for btrfs-restore

[Eric Sandeen]

On 8/20/14, 10:35 PM, Gui Hecheng wrote:
> A memory problem reported by valgrind as follows:
> 	=== Syscall param pwrite64(buf) points to uninitialised byte(s)
> When running:
> 	# valgrind --leak-check=yes btrfs restore /dev/sda9 /mnt/backup
> 
> Because the output buf size is alloced with malloc, but the length of
> output data is shorter than the sizeof(buf), so valgrind report
> uninitialised byte(s).
> We could use calloc to repalce malloc and clear this WARNING away.
> 
> Reported-by: Marc Dietrich <marvin24@gmx.de>
> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>

Sorry for the noise in reply to this, I think the patch itself
is fine.  It would have clarified things to the reviewer, though,
if you had added more detail to the commit log, such as:

> If a btrfs-restore process encounters corruption and fails
> to properly decompress all data, some parts of the output
> buffer may not be initialized.  This was reported by valgrind as
> follows:
> 	=== Syscall param pwrite64(buf) points to uninitialised byte(s)
> When running:
> 	# valgrind --leak-check=yes btrfs restore /dev/sda9 /mnt/backup
> 
> Because the output buf size is alloced with malloc, but the length of
> output data is shorter than the sizeof(buf), so valgrind reports
> uninitialised byte(s).
>
> We could use calloc to replace malloc to ensure that all written
> bytes are initialized.

Anyway:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Thanks,
-Eric

> ---
>  cmds-restore.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds-restore.c b/cmds-restore.c
> index cbda6bb..bb72311 100644
> --- a/cmds-restore.c
> +++ b/cmds-restore.c
> @@ -251,7 +251,7 @@ static int copy_one_inline(int fd, struct btrfs_path *path, u64 pos)
>  	}
>  
>  	ram_size = btrfs_file_extent_ram_bytes(leaf, fi);
> -	outbuf = malloc(ram_size);
> +	outbuf = calloc(1, ram_size);
>  	if (!outbuf) {
>  		fprintf(stderr, "No memory\n");
>  		return -ENOMEM;
> @@ -320,7 +320,7 @@ static int copy_one_extent(struct btrfs_root *root, int fd,
>  	}
>  
>  	if (compress != BTRFS_COMPRESS_NONE) {
> -		outbuf = malloc(ram_size);
> +		outbuf = calloc(1, ram_size);
>  		if (!outbuf) {
>  			fprintf(stderr, "No memory\n");
>  			free(inbuf);
>