[PATCH] debugfs: fix memory leak problem in read_list()

[PATCH] debugfs: fix memory leak problem in read_list()

[Zhiqiang Liu]

In read_list func, if strtoull() fails in while loop,
we will return the error code directly. Then, memory of
variable lst will be leaked without setting to *list.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: linfeilong <linfeilong@huawei.com>
---
 debugfs/util.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/debugfs/util.c b/debugfs/util.c
index be6b550e..9e880548 100644
--- a/debugfs/util.c
+++ b/debugfs/util.c
@@ -530,12 +530,16 @@ errcode_t read_list(char *str, blk64_t **list, size_t *len)

 		errno = 0;
 		y = x = strtoull(tok, &e, 0);
-		if (errno)
-			return errno;
+		if (errno) {
+			retval = errno;
+			break;
+		}
 		if (*e == '-') {
 			y = strtoull(e + 1, NULL, 0);
-			if (errno)
-				return errno;
+			if (errno) {
+				retval = errno;
+				break;
+			}
 		} else if (*e != 0) {
 			retval = EINVAL;
 			break;
-- 
2.19.1

Re: [PATCH] debugfs: fix memory leak problem in read_list()

[Zhiqiang Liu]

friendly ping ...

On 2021/2/20 16:41, Zhiqiang Liu wrote:
> 
> In read_list func, if strtoull() fails in while loop,
> we will return the error code directly. Then, memory of
> variable lst will be leaked without setting to *list.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: linfeilong <linfeilong@huawei.com>
> ---
>  debugfs/util.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/debugfs/util.c b/debugfs/util.c
> index be6b550e..9e880548 100644
> --- a/debugfs/util.c
> +++ b/debugfs/util.c
> @@ -530,12 +530,16 @@ errcode_t read_list(char *str, blk64_t **list, size_t *len)
> 
>  		errno = 0;
>  		y = x = strtoull(tok, &e, 0);
> -		if (errno)
> -			return errno;
> +		if (errno) {
> +			retval = errno;
> +			break;
> +		}
>  		if (*e == '-') {
>  			y = strtoull(e + 1, NULL, 0);
> -			if (errno)
> -				return errno;
> +			if (errno) {
> +				retval = errno;
> +				break;
> +			}
>  		} else if (*e != 0) {
>  			retval = EINVAL;
>  			break;
> 

Re: [PATCH] debugfs: fix memory leak problem in read_list()

[harshad shirwadkar]

On Sat, Feb 20, 2021 at 12:41 AM Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
>
>
> In read_list func, if strtoull() fails in while loop,
> we will return the error code directly. Then, memory of
> variable lst will be leaked without setting to *list.
>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: linfeilong <linfeilong@huawei.com>
> ---
>  debugfs/util.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/debugfs/util.c b/debugfs/util.c
> index be6b550e..9e880548 100644
> --- a/debugfs/util.c
> +++ b/debugfs/util.c
> @@ -530,12 +530,16 @@ errcode_t read_list(char *str, blk64_t **list, size_t *len)
>
>                 errno = 0;
>                 y = x = strtoull(tok, &e, 0);
> -               if (errno)
> -                       return errno;
> +               if (errno) {
> +                       retval = errno;
> +                       break;
> +               }
Shouldn't we have `goto err;` here instead of break? strtoull failure
here indicates that no valid value was found, so instead of returning
the allocated memory, we should just free the memory and return error.

- Harshad
>                 if (*e == '-') {
>                         y = strtoull(e + 1, NULL, 0);
> -                       if (errno)
> -                               return errno;
> +                       if (errno) {
> +                               retval = errno;
> +                               break;
> +                       }
>                 } else if (*e != 0) {
>                         retval = EINVAL;
>                         break;
> --
> 2.19.1
>
>

Re: [PATCH] debugfs: fix memory leak problem in read_list()

[Theodore Ts'o]

On Thu, Feb 25, 2021 at 07:51:09AM -0800, harshad shirwadkar wrote:
> On Sat, Feb 20, 2021 at 12:41 AM Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
> >
> >
> > In read_list func, if strtoull() fails in while loop,
> > we will return the error code directly. Then, memory of
> > variable lst will be leaked without setting to *list.
> >
> > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> > Signed-off-by: linfeilong <linfeilong@huawei.com>
> > ---
> >  debugfs/util.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/debugfs/util.c b/debugfs/util.c
> > index be6b550e..9e880548 100644
> > --- a/debugfs/util.c
> > +++ b/debugfs/util.c
> > @@ -530,12 +530,16 @@ errcode_t read_list(char *str, blk64_t **list, size_t *len)
> >
> >                 errno = 0;
> >                 y = x = strtoull(tok, &e, 0);
> > -               if (errno)
> > -                       return errno;
> > +               if (errno) {
> > +                       retval = errno;
> > +                       break;
> > +               }
> Shouldn't we have `goto err;` here instead of break? strtoull failure
> here indicates that no valid value was found, so instead of returning
> the allocated memory, we should just free the memory and return error.

As of commit 462c424500a5 ("debugfs: fix memory allocation failures
when parsing journal_write arguments") there is no longer the err:
goto target.  The goal is to move to a model where the caller is
exclusively responsible for freeing any allocated memory, since if
realloc() has gotten into the act, the memory pointed to in *list
would have been freed by realloc().  The fix is to make sure *list is
updated before we return.  This also allows the caller to have access
to the list of numbers parsed before we ran into an error.

So the Zhiqiang's patch is correc, and I will apply it.

       		  	   	       	 - Ted

Re: [PATCH] debugfs: fix memory leak problem in read_list()

[harshad shirwadkar]

Thanks for the clarification, I think my repo was a bit stale.

- Harshad

On Thu, Feb 25, 2021 at 9:02 AM Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Thu, Feb 25, 2021 at 07:51:09AM -0800, harshad shirwadkar wrote:
> > On Sat, Feb 20, 2021 at 12:41 AM Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
> > >
> > >
> > > In read_list func, if strtoull() fails in while loop,
> > > we will return the error code directly. Then, memory of
> > > variable lst will be leaked without setting to *list.
> > >
> > > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> > > Signed-off-by: linfeilong <linfeilong@huawei.com>
> > > ---
> > >  debugfs/util.c | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/debugfs/util.c b/debugfs/util.c
> > > index be6b550e..9e880548 100644
> > > --- a/debugfs/util.c
> > > +++ b/debugfs/util.c
> > > @@ -530,12 +530,16 @@ errcode_t read_list(char *str, blk64_t **list, size_t *len)
> > >
> > >                 errno = 0;
> > >                 y = x = strtoull(tok, &e, 0);
> > > -               if (errno)
> > > -                       return errno;
> > > +               if (errno) {
> > > +                       retval = errno;
> > > +                       break;
> > > +               }
> > Shouldn't we have `goto err;` here instead of break? strtoull failure
> > here indicates that no valid value was found, so instead of returning
> > the allocated memory, we should just free the memory and return error.
>
> As of commit 462c424500a5 ("debugfs: fix memory allocation failures
> when parsing journal_write arguments") there is no longer the err:
> goto target.  The goal is to move to a model where the caller is
> exclusively responsible for freeing any allocated memory, since if
> realloc() has gotten into the act, the memory pointed to in *list
> would have been freed by realloc().  The fix is to make sure *list is
> updated before we return.  This also allows the caller to have access
> to the list of numbers parsed before we ran into an error.
>
> So the Zhiqiang's patch is correc, and I will apply it.
>
>                                          - Ted

Re: [PATCH] debugfs: fix memory leak problem in read_list()

[Zhiqiang Liu]

Thank you and harshad shirwadkar.

On 2021/2/26 1:02, Theodore Ts'o wrote:
> On Thu, Feb 25, 2021 at 07:51:09AM -0800, harshad shirwadkar wrote:
>> On Sat, Feb 20, 2021 at 12:41 AM Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote:
>>>
>>>
>>> In read_list func, if strtoull() fails in while loop,
>>> we will return the error code directly. Then, memory of
>>> variable lst will be leaked without setting to *list.
>>>
>>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>>> Signed-off-by: linfeilong <linfeilong@huawei.com>
>>> ---
>>>  debugfs/util.c | 12 ++++++++----
>>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/debugfs/util.c b/debugfs/util.c
>>> index be6b550e..9e880548 100644
>>> --- a/debugfs/util.c
>>> +++ b/debugfs/util.c
>>> @@ -530,12 +530,16 @@ errcode_t read_list(char *str, blk64_t **list, size_t *len)
>>>
>>>                 errno = 0;
>>>                 y = x = strtoull(tok, &e, 0);
>>> -               if (errno)
>>> -                       return errno;
>>> +               if (errno) {
>>> +                       retval = errno;
>>> +                       break;
>>> +               }
>> Shouldn't we have `goto err;` here instead of break? strtoull failure
>> here indicates that no valid value was found, so instead of returning
>> the allocated memory, we should just free the memory and return error.
> 
> As of commit 462c424500a5 ("debugfs: fix memory allocation failures
> when parsing journal_write arguments") there is no longer the err:
> goto target.  The goal is to move to a model where the caller is
> exclusively responsible for freeing any allocated memory, since if
> realloc() has gotten into the act, the memory pointed to in *list
> would have been freed by realloc().  The fix is to make sure *list is
> updated before we return.  This also allows the caller to have access
> to the list of numbers parsed before we ran into an error.
> 
> So the Zhiqiang's patch is correc, and I will apply it.
> 
>        		  	   	       	 - Ted
> 
> .
>