diff mbox series

[1/3] misc: fix valgrind complaints

Message ID 161017372088.1142776.17470250928392025583.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series various: random fixes | expand

Commit Message

Darrick J. Wong Jan. 9, 2021, 6:28 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Zero the memory that we pass to the kernel via ioctls so that we never
pass userspace heap/stack garbage around.  This silences valgrind
complaints about uninitialized padding areas.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 libhandle/handle.c |    7 ++++++-
 scrub/inodes.c     |    1 +
 scrub/spacemap.c   |    2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

Comments

Chandan Babu R Jan. 11, 2021, 1:38 p.m. UTC | #1 
On 09 Jan 2021 at 11:58, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Zero the memory that we pass to the kernel via ioctls so that we never
> pass userspace heap/stack garbage around.  This silences valgrind
> complaints about uninitialized padding areas.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  libhandle/handle.c |    7 ++++++-
>  scrub/inodes.c     |    1 +
>  scrub/spacemap.c   |    2 +-
>  3 files changed, 8 insertions(+), 2 deletions(-)
>
>
> diff --git a/libhandle/handle.c b/libhandle/handle.c
> index 5c1686b3..a6b35b09 100644
> --- a/libhandle/handle.c
> +++ b/libhandle/handle.c
> @@ -235,9 +235,12 @@ obj_to_handle(
>  {
>  	char		hbuf [MAXHANSIZ];
>  	int		ret;
> -	uint32_t	handlen;
> +	uint32_t	handlen = 0;
>  	xfs_fsop_handlereq_t hreq;
>
> +	memset(&hreq, 0, sizeof(hreq));
> +	memset(hbuf, 0, MAXHANSIZ);
> +
>  	if (opcode == XFS_IOC_FD_TO_HANDLE) {
>  		hreq.fd      = obj.fd;
>  		hreq.path    = NULL;
> @@ -280,6 +283,7 @@ open_by_fshandle(
>  	if ((fsfd = handle_to_fsfd(fshanp, &path)) < 0)
>  		return -1;
>
> +	memset(&hreq, 0, sizeof(hreq));
>  	hreq.fd       = 0;
>  	hreq.path     = NULL;
>  	hreq.oflags   = rw | O_LARGEFILE;
> @@ -387,6 +391,7 @@ attr_list_by_handle(
>  	if ((fd = handle_to_fsfd(hanp, &path)) < 0)
>  		return -1;
>
> +	memset(&alhreq, 0, sizeof(alhreq));
>  	alhreq.hreq.fd       = 0;
>  	alhreq.hreq.path     = NULL;
>  	alhreq.hreq.oflags   = O_LARGEFILE;
> diff --git a/scrub/inodes.c b/scrub/inodes.c
> index 4550db83..f2bce16f 100644
> --- a/scrub/inodes.c
> +++ b/scrub/inodes.c
> @@ -129,6 +129,7 @@ scan_ag_inodes(
>  				minor(ctx->fsinfo.fs_datadev),
>  				agno);
>
> +	memset(&handle, 0, sizeof(handle));
>  	memcpy(&handle.ha_fsid, ctx->fshandle, sizeof(handle.ha_fsid));
>  	handle.ha_fid.fid_len = sizeof(xfs_fid_t) -
>  			sizeof(handle.ha_fid.fid_len);
> diff --git a/scrub/spacemap.c b/scrub/spacemap.c
> index 9653916d..9362710e 100644
> --- a/scrub/spacemap.c
> +++ b/scrub/spacemap.c
> @@ -47,7 +47,7 @@ scrub_iterate_fsmap(
>  	int			i;
>  	int			error;
>
> -	head = malloc(fsmap_sizeof(FSMAP_NR));
> +	head = calloc(1, fsmap_sizeof(FSMAP_NR));
>  	if (!head)
>  		return errno;
>

Minor nit: The "memset(head, 0, sizeof(*head))" statement following the above
call to calloc() can now be removed.

--
chandan
Christoph Hellwig Jan. 11, 2021, 5:27 p.m. UTC | #2 
On Fri, Jan 08, 2021 at 10:28:40PM -0800, Darrick J. Wong wrote:
>  	char		hbuf [MAXHANSIZ];
>  	int		ret;
> -	uint32_t	handlen;
> +	uint32_t	handlen = 0;
>  	xfs_fsop_handlereq_t hreq;
>  
> +	memset(&hreq, 0, sizeof(hreq));
> +	memset(hbuf, 0, MAXHANSIZ);

Using empty initializers at declaration time is simpler and sometimes
more efficient.  But either way will work fine.
Darrick J. Wong Jan. 12, 2021, 1:22 a.m. UTC | #3 
On Mon, Jan 11, 2021 at 07:08:27PM +0530, Chandan Babu R wrote:
> 
> On 09 Jan 2021 at 11:58, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Zero the memory that we pass to the kernel via ioctls so that we never
> > pass userspace heap/stack garbage around.  This silences valgrind
> > complaints about uninitialized padding areas.
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  libhandle/handle.c |    7 ++++++-
> >  scrub/inodes.c     |    1 +
> >  scrub/spacemap.c   |    2 +-
> >  3 files changed, 8 insertions(+), 2 deletions(-)
> >
> >
> > diff --git a/libhandle/handle.c b/libhandle/handle.c
> > index 5c1686b3..a6b35b09 100644
> > --- a/libhandle/handle.c
> > +++ b/libhandle/handle.c
> > @@ -235,9 +235,12 @@ obj_to_handle(
> >  {
> >  	char		hbuf [MAXHANSIZ];
> >  	int		ret;
> > -	uint32_t	handlen;
> > +	uint32_t	handlen = 0;
> >  	xfs_fsop_handlereq_t hreq;
> >
> > +	memset(&hreq, 0, sizeof(hreq));
> > +	memset(hbuf, 0, MAXHANSIZ);
> > +
> >  	if (opcode == XFS_IOC_FD_TO_HANDLE) {
> >  		hreq.fd      = obj.fd;
> >  		hreq.path    = NULL;
> > @@ -280,6 +283,7 @@ open_by_fshandle(
> >  	if ((fsfd = handle_to_fsfd(fshanp, &path)) < 0)
> >  		return -1;
> >
> > +	memset(&hreq, 0, sizeof(hreq));
> >  	hreq.fd       = 0;
> >  	hreq.path     = NULL;
> >  	hreq.oflags   = rw | O_LARGEFILE;
> > @@ -387,6 +391,7 @@ attr_list_by_handle(
> >  	if ((fd = handle_to_fsfd(hanp, &path)) < 0)
> >  		return -1;
> >
> > +	memset(&alhreq, 0, sizeof(alhreq));
> >  	alhreq.hreq.fd       = 0;
> >  	alhreq.hreq.path     = NULL;
> >  	alhreq.hreq.oflags   = O_LARGEFILE;
> > diff --git a/scrub/inodes.c b/scrub/inodes.c
> > index 4550db83..f2bce16f 100644
> > --- a/scrub/inodes.c
> > +++ b/scrub/inodes.c
> > @@ -129,6 +129,7 @@ scan_ag_inodes(
> >  				minor(ctx->fsinfo.fs_datadev),
> >  				agno);
> >
> > +	memset(&handle, 0, sizeof(handle));
> >  	memcpy(&handle.ha_fsid, ctx->fshandle, sizeof(handle.ha_fsid));
> >  	handle.ha_fid.fid_len = sizeof(xfs_fid_t) -
> >  			sizeof(handle.ha_fid.fid_len);
> > diff --git a/scrub/spacemap.c b/scrub/spacemap.c
> > index 9653916d..9362710e 100644
> > --- a/scrub/spacemap.c
> > +++ b/scrub/spacemap.c
> > @@ -47,7 +47,7 @@ scrub_iterate_fsmap(
> >  	int			i;
> >  	int			error;
> >
> > -	head = malloc(fsmap_sizeof(FSMAP_NR));
> > +	head = calloc(1, fsmap_sizeof(FSMAP_NR));
> >  	if (!head)
> >  		return errno;
> >
> 
> Minor nit: The "memset(head, 0, sizeof(*head))" statement following the above
> call to calloc() can now be removed.

FIxed, thanks.

--D

> --
> chandan
Darrick J. Wong Jan. 12, 2021, 1:22 a.m. UTC | #4 
On Mon, Jan 11, 2021 at 05:27:46PM +0000, Christoph Hellwig wrote:
> On Fri, Jan 08, 2021 at 10:28:40PM -0800, Darrick J. Wong wrote:
> >  	char		hbuf [MAXHANSIZ];
> >  	int		ret;
> > -	uint32_t	handlen;
> > +	uint32_t	handlen = 0;
> >  	xfs_fsop_handlereq_t hreq;
> >  
> > +	memset(&hreq, 0, sizeof(hreq));
> > +	memset(hbuf, 0, MAXHANSIZ);
> 
> Using empty initializers at declaration time is simpler and sometimes
> more efficient.  But either way will work fine.

I'll fix that then, and get rid of two more typedef usages.

--D
diff mbox series

Patch

diff --git a/libhandle/handle.c b/libhandle/handle.c
index 5c1686b3..a6b35b09 100644
--- a/libhandle/handle.c
+++ b/libhandle/handle.c
@@ -235,9 +235,12 @@  obj_to_handle(
 {
 	char		hbuf [MAXHANSIZ];
 	int		ret;
-	uint32_t	handlen;
+	uint32_t	handlen = 0;
 	xfs_fsop_handlereq_t hreq;
 
+	memset(&hreq, 0, sizeof(hreq));
+	memset(hbuf, 0, MAXHANSIZ);
+
 	if (opcode == XFS_IOC_FD_TO_HANDLE) {
 		hreq.fd      = obj.fd;
 		hreq.path    = NULL;
@@ -280,6 +283,7 @@  open_by_fshandle(
 	if ((fsfd = handle_to_fsfd(fshanp, &path)) < 0)
 		return -1;
 
+	memset(&hreq, 0, sizeof(hreq));
 	hreq.fd       = 0;
 	hreq.path     = NULL;
 	hreq.oflags   = rw | O_LARGEFILE;
@@ -387,6 +391,7 @@  attr_list_by_handle(
 	if ((fd = handle_to_fsfd(hanp, &path)) < 0)
 		return -1;
 
+	memset(&alhreq, 0, sizeof(alhreq));
 	alhreq.hreq.fd       = 0;
 	alhreq.hreq.path     = NULL;
 	alhreq.hreq.oflags   = O_LARGEFILE;
diff --git a/scrub/inodes.c b/scrub/inodes.c
index 4550db83..f2bce16f 100644
--- a/scrub/inodes.c
+++ b/scrub/inodes.c
@@ -129,6 +129,7 @@  scan_ag_inodes(
 				minor(ctx->fsinfo.fs_datadev),
 				agno);
 
+	memset(&handle, 0, sizeof(handle));
 	memcpy(&handle.ha_fsid, ctx->fshandle, sizeof(handle.ha_fsid));
 	handle.ha_fid.fid_len = sizeof(xfs_fid_t) -
 			sizeof(handle.ha_fid.fid_len);
diff --git a/scrub/spacemap.c b/scrub/spacemap.c
index 9653916d..9362710e 100644
--- a/scrub/spacemap.c
+++ b/scrub/spacemap.c
@@ -47,7 +47,7 @@  scrub_iterate_fsmap(
 	int			i;
 	int			error;
 
-	head = malloc(fsmap_sizeof(FSMAP_NR));
+	head = calloc(1, fsmap_sizeof(FSMAP_NR));
 	if (!head)
 		return errno;