[ndctl] libdaxctl: fix memory leaks with daxctl_memory objects
diff mbox series

Message ID 20191001173726.21846-1-vishal.l.verma@intel.com
State Accepted
Commit 7438ce43d0371a3b90f333a93a85dcfa2799b4d9
Headers show
Series
  • [ndctl] libdaxctl: fix memory leaks with daxctl_memory objects
Related show

Commit Message

Verma, Vishal L Oct. 1, 2019, 5:37 p.m. UTC
The daxctl_dev_alloc_mem() helper which is used to instantiate a new
memory object did so, but neglected to attach the memory object to the
parent 'dev' object. As a result, every invocation of
'daxctl_dev_get_memory() resulted in a new, orphan memory object being
created, which also resulted in libdaxctl leaking memory.

Fix the parent association for 'mem' objects, and in free_mem, remove
the check for 'dev' being present - mem objects will always associated
with a dev.

Additionally, we were neglecting to free 'mem->mem_buf' in free_mem, so
fix this up as well.

Fixes: e8bf803e359b ("libdaxctl: add a 'daxctl_memory' object for memory based operations")
Link: https://github.com/pmem/ndctl/issues/112
Reported-by: Michal Biesek <michal.biesek@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 daxctl/lib/libdaxctl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Ira Weiny Oct. 1, 2019, 11:19 p.m. UTC | #1
On Tue, Oct 01, 2019 at 11:37:26AM -0600, 'Vishal Verma' wrote:
> The daxctl_dev_alloc_mem() helper which is used to instantiate a new
> memory object did so, but neglected to attach the memory object to the
> parent 'dev' object. As a result, every invocation of
> 'daxctl_dev_get_memory() resulted in a new, orphan memory object being
> created, which also resulted in libdaxctl leaking memory.
> 
> Fix the parent association for 'mem' objects, and in free_mem, remove
> the check for 'dev' being present - mem objects will always associated
> with a dev.
> 
> Additionally, we were neglecting to free 'mem->mem_buf' in free_mem, so
> fix this up as well.
> 
> Fixes: e8bf803e359b ("libdaxctl: add a 'daxctl_memory' object for memory based operations")
> Link: https://github.com/pmem/ndctl/issues/112
> Reported-by: Michal Biesek <michal.biesek@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Minor NIT below.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  daxctl/lib/libdaxctl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
> index 8abfd64..639224c 100644
> --- a/daxctl/lib/libdaxctl.c
> +++ b/daxctl/lib/libdaxctl.c
> @@ -204,8 +204,9 @@ DAXCTL_EXPORT void daxctl_region_get_uuid(struct daxctl_region *region, uuid_t u
>  
>  static void free_mem(struct daxctl_dev *dev)
>  {
> -	if (dev && dev->mem) {
> +	if (dev->mem) {

There is a comment in daxctl_dev_disable() which says:

        /* If there is a memory object, first free that */

I'm 100% sure that dev can't be NULL there.  So that comment no longer applies.

May want to remove that comment.

Ira

>  		free(dev->mem->node_path);
> +		free(dev->mem->mem_buf);
>  		free(dev->mem);
>  		dev->mem = NULL;
>  	}
> @@ -450,6 +451,7 @@ static struct daxctl_memory *daxctl_dev_alloc_mem(struct daxctl_dev *dev)
>  		goto err_node;
>  	mem->buf_len = strlen(node_base) + 256;
>  
> +	dev->mem = mem;
>  	return mem;
>  
>  err_node:
> -- 
> 2.20.1
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
Verma, Vishal L Oct. 3, 2019, 12:01 a.m. UTC | #2
On Tue, 2019-10-01 at 16:19 -0700, Ira Weiny wrote:
> 
> > diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
> > index 8abfd64..639224c 100644
> > --- a/daxctl/lib/libdaxctl.c
> > +++ b/daxctl/lib/libdaxctl.c
> > @@ -204,8 +204,9 @@ DAXCTL_EXPORT void daxctl_region_get_uuid(struct daxctl_region *region, uuid_t u
> >  
> >  static void free_mem(struct daxctl_dev *dev)
> >  {
> > -	if (dev && dev->mem) {
> > +	if (dev->mem) {
> 
> There is a comment in daxctl_dev_disable() which says:
> 
>         /* If there is a memory object, first free that */
> 
> I'm 100% sure that dev can't be NULL there.  So that comment no longer applies.
> 
> May want to remove that comment.
> 
Hm, I'm not sure I follow.

Yes, dev can't be null there, but 'mem' can me. Hence the comment saying
if /mem/ is present, free it.

The check in free_mem() only checks for non-NULL mem too.
Ira Weiny Oct. 3, 2019, 3:41 p.m. UTC | #3
On Wed, Oct 02, 2019 at 05:01:24PM -0700, 'Vishal Verma' wrote:
> On Tue, 2019-10-01 at 16:19 -0700, Ira Weiny wrote:
> > 
> > > diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
> > > index 8abfd64..639224c 100644
> > > --- a/daxctl/lib/libdaxctl.c
> > > +++ b/daxctl/lib/libdaxctl.c
> > > @@ -204,8 +204,9 @@ DAXCTL_EXPORT void daxctl_region_get_uuid(struct daxctl_region *region, uuid_t u
> > >  
> > >  static void free_mem(struct daxctl_dev *dev)
> > >  {
> > > -	if (dev && dev->mem) {
> > > +	if (dev->mem) {
> > 
> > There is a comment in daxctl_dev_disable() which says:
> > 
> >         /* If there is a memory object, first free that */
> > 
> > I'm 100% sure that dev can't be NULL there.  So that comment no longer applies.
> > 
> > May want to remove that comment.
> > 
> Hm, I'm not sure I follow.
> 
> Yes, dev can't be null there, but 'mem' can me. Hence the comment saying
> if /mem/ is present, free it.
> 
> The check in free_mem() only checks for non-NULL mem too.

Ah yea ok.  Sorry
Ira

Patch
diff mbox series

diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index 8abfd64..639224c 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -204,8 +204,9 @@  DAXCTL_EXPORT void daxctl_region_get_uuid(struct daxctl_region *region, uuid_t u
 
 static void free_mem(struct daxctl_dev *dev)
 {
-	if (dev && dev->mem) {
+	if (dev->mem) {
 		free(dev->mem->node_path);
+		free(dev->mem->mem_buf);
 		free(dev->mem);
 		dev->mem = NULL;
 	}
@@ -450,6 +451,7 @@  static struct daxctl_memory *daxctl_dev_alloc_mem(struct daxctl_dev *dev)
 		goto err_node;
 	mem->buf_len = strlen(node_base) + 256;
 
+	dev->mem = mem;
 	return mem;
 
 err_node: