| Message ID | 1408592136-7606-1-git-send-email-guihc.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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
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 -- 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-info.html
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); > -- 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-info.html
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 -- 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-info.html
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
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 -- 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-info.html
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); > -- 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-info.html
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);
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(-)