[PATCH] fdtget.c: Fix memory leak

[PATCH] fdtget.c: Fix memory leak

[Jean-Christophe Dubois]

CID 132823 (#1 of 1): Resource leak (RESOURCE_LEAK)
5. leaked_storage: Variable blob going out of scope leaks the storage it points to.

Signed-off-by: Jean-Christophe Dubois <jcd-WBS85hRCVJbxB9160cZjhg@public.gmane.org>
---
 fdtget.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fdtget.c b/fdtget.c
index 4377419..fb9d0e1 100644
--- a/fdtget.c
+++ b/fdtget.c
@@ -266,14 +266,20 @@ static int do_fdtget(struct display_info *disp, const char *filename,
 				continue;
 			} else {
 				report_error(arg[i], node);
+				free(blob);
 				return -1;
 			}
 		}
 		prop = args_per_step == 1 ? NULL : arg[i + 1];
 
-		if (show_data_for_item(blob, disp, node, prop))
+		if (show_data_for_item(blob, disp, node, prop)) {
+			free(blob);
 			return -1;
+		}
 	}
+
+	free(blob);
+
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH] fdtget.c: Fix memory leak

[David Gibson]

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

On Wed, Jul 13, 2016 at 12:36:08AM +0200, Jean-Christophe Dubois wrote:
> CID 132823 (#1 of 1): Resource leak (RESOURCE_LEAK)
> 5. leaked_storage: Variable blob going out of scope leaks the storage it points to.
> 
> Signed-off-by: Jean-Christophe Dubois <jcd-WBS85hRCVJbxB9160cZjhg@public.gmane.org>

Since the program exits immediately after this free(), there's really
no point.

> ---
>  fdtget.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fdtget.c b/fdtget.c
> index 4377419..fb9d0e1 100644
> --- a/fdtget.c
> +++ b/fdtget.c
> @@ -266,14 +266,20 @@ static int do_fdtget(struct display_info *disp, const char *filename,
>  				continue;
>  			} else {
>  				report_error(arg[i], node);
> +				free(blob);
>  				return -1;
>  			}
>  		}
>  		prop = args_per_step == 1 ? NULL : arg[i + 1];
>  
> -		if (show_data_for_item(blob, disp, node, prop))
> +		if (show_data_for_item(blob, disp, node, prop)) {
> +			free(blob);
>  			return -1;
> +		}
>  	}
> +
> +	free(blob);
> +
>  	return 0;
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] fdtget.c: Fix memory leak

[Jean-Christophe DUBOIS]

Le 13/07/2016 04:50, David Gibson a écrit :
> On Wed, Jul 13, 2016 at 12:36:08AM +0200, Jean-Christophe Dubois wrote:
>> CID 132823 (#1 of 1): Resource leak (RESOURCE_LEAK)
>> 5. leaked_storage: Variable blob going out of scope leaks the storage it points to.
>>
>> Signed-off-by: Jean-Christophe Dubois <jcd-WBS85hRCVJbxB9160cZjhg@public.gmane.org>
> Since the program exits immediately after this free(), there's really
> no point.
This is usually a good practice to free explicitly the memory we are 
allocating rather than relying on some kind of (final) garbage collector 
(like in java).

It seems cheap to fix but this is your call.
>
>> ---
>>   fdtget.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/fdtget.c b/fdtget.c
>> index 4377419..fb9d0e1 100644
>> --- a/fdtget.c
>> +++ b/fdtget.c
>> @@ -266,14 +266,20 @@ static int do_fdtget(struct display_info *disp, const char *filename,
>>   				continue;
>>   			} else {
>>   				report_error(arg[i], node);
>> +				free(blob);
>>   				return -1;
>>   			}
>>   		}
>>   		prop = args_per_step == 1 ? NULL : arg[i + 1];
>>   
>> -		if (show_data_for_item(blob, disp, node, prop))
>> +		if (show_data_for_item(blob, disp, node, prop)) {
>> +			free(blob);
>>   			return -1;
>> +		}
>>   	}
>> +
>> +	free(blob);
>> +
>>   	return 0;
>>   }
>>   

Re: [PATCH] fdtget.c: Fix memory leak

[David Gibson]

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

On Wed, Jul 13, 2016 at 08:05:16AM +0200, Jean-Christophe DUBOIS wrote:
> Le 13/07/2016 04:50, David Gibson a écrit :
> > On Wed, Jul 13, 2016 at 12:36:08AM +0200, Jean-Christophe Dubois wrote:
> > > CID 132823 (#1 of 1): Resource leak (RESOURCE_LEAK)
> > > 5. leaked_storage: Variable blob going out of scope leaks the storage it points to.
> > > 
> > > Signed-off-by: Jean-Christophe Dubois <jcd-WBS85hRCVJbxB9160cZjhg@public.gmane.org>
> > Since the program exits immediately after this free(), there's really
> > no point.
> This is usually a good practice to free explicitly the memory we are
> allocating rather than relying on some kind of (final) garbage collector
> (like in java).

Well... it's important to have a consistent model for memory
management and stick to it.  For little programs like this, never
freeing - essentially using the process lifetime as a really simple
pool allocator - is a pretty valid option.

That said, I guess there are existing free()s so it's not really using
that model consistently.  So, yeah, ok, we might as well shut the
checker up by applying these.

> 
> It seems cheap to fix but this is your call.
> > 
> > > ---
> > >   fdtget.c | 8 +++++++-
> > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fdtget.c b/fdtget.c
> > > index 4377419..fb9d0e1 100644
> > > --- a/fdtget.c
> > > +++ b/fdtget.c
> > > @@ -266,14 +266,20 @@ static int do_fdtget(struct display_info *disp, const char *filename,
> > >   				continue;
> > >   			} else {
> > >   				report_error(arg[i], node);
> > > +				free(blob);
> > >   				return -1;
> > >   			}
> > >   		}
> > >   		prop = args_per_step == 1 ? NULL : arg[i + 1];
> > > -		if (show_data_for_item(blob, disp, node, prop))
> > > +		if (show_data_for_item(blob, disp, node, prop)) {
> > > +			free(blob);
> > >   			return -1;
> > > +		}
> > >   	}
> > > +
> > > +	free(blob);
> > > +
> > >   	return 0;
> > >   }
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]