2019-08-15 18:27:31

by Wenwen Wang

Subject: [PATCH] wimax/i2400m: fix a memory leak bug

In i2400m_barker_db_init(), 'options_orig' is allocated through kstrdup()
to hold the original command line options. Then, the options are parsed.
However, if an error occurs during the parsing process, 'options_orig' is
not deallocated, leading to a memory leak bug. To fix this issue, free
'options_orig' before returning the error.

Signed-off-by: Wenwen Wang <[email protected]>
---
drivers/net/wimax/i2400m/fw.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wimax/i2400m/fw.c b/drivers/net/wimax/i2400m/fw.c
index e9fc168..6b36f6d 100644
--- a/drivers/net/wimax/i2400m/fw.c
+++ b/drivers/net/wimax/i2400m/fw.c
@@ -342,6 +342,7 @@ int i2400m_barker_db_init(const char *_options)
"a 32-bit number\n",
__func__, token);
result = -EINVAL;
+ kfree(options_orig);
goto error_parse;
}
if (barker == 0) {
@@ -350,8 +351,10 @@ int i2400m_barker_db_init(const char *_options)
continue;
}
result = i2400m_barker_db_add(barker);
- if (result < 0)
+ if (result < 0) {
+ kfree(options_orig);
goto error_add;
+ }
}
kfree(options_orig);
}
--
2.7.4

2019-08-15 20:10:52

by Liam R. Howlett

Subject: Re: [PATCH] wimax/i2400m: fix a memory leak bug

* Wenwen Wang <[email protected]> [190815 14:05]:
> In i2400m_barker_db_init(), 'options_orig' is allocated through kstrdup()
> to hold the original command line options. Then, the options are parsed.
> However, if an error occurs during the parsing process, 'options_orig' is
> not deallocated, leading to a memory leak bug. To fix this issue, free
> 'options_orig' before returning the error.
>
> Signed-off-by: Wenwen Wang <[email protected]>
> ---
> drivers/net/wimax/i2400m/fw.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wimax/i2400m/fw.c b/drivers/net/wimax/i2400m/fw.c
> index e9fc168..6b36f6d 100644
> --- a/drivers/net/wimax/i2400m/fw.c
> +++ b/drivers/net/wimax/i2400m/fw.c
> @@ -342,6 +342,7 @@ int i2400m_barker_db_init(const char *_options)
> "a 32-bit number\n",
> __func__, token);
> result = -EINVAL;
> + kfree(options_orig);
> goto error_parse;
> }
> if (barker == 0) {
> @@ -350,8 +351,10 @@ int i2400m_barker_db_init(const char *_options)
> continue;
> }
> result = i2400m_barker_db_add(barker);
> - if (result < 0)
> + if (result < 0) {
> + kfree(options_orig);
> goto error_add;

I know that you didn't add this error_add label, but it seems like the
incorrect goto label. Although looking at the caller indicates an add
failed, this label is used prior to and after the memory leak you are
trying to fix. It might be better to change this label to something
like error_parse_add and move the kfree to the unwinding. If a new
label is used, it becomes more clear as to what is being undone and
there aren't two jumps into an unwind from two very different stages of
the function. Adding a new label also has the benefit of moving the
kfree to the unwind of error_parse.

Thanks,
Liam


> + }
> }
> kfree(options_orig);
> }
> --
> 2.7.4
>

2019-08-15 20:43:20

by Wenwen Wang

Subject: Re: [PATCH] wimax/i2400m: fix a memory leak bug

On Thu, Aug 15, 2019 at 2:45 PM Liam R. Howlett <[email protected]> wrote:
>
> * Wenwen Wang <[email protected]> [190815 14:05]:
> > In i2400m_barker_db_init(), 'options_orig' is allocated through kstrdup()
> > to hold the original command line options. Then, the options are parsed.
> > However, if an error occurs during the parsing process, 'options_orig' is
> > not deallocated, leading to a memory leak bug. To fix this issue, free
> > 'options_orig' before returning the error.
> >
> > Signed-off-by: Wenwen Wang <[email protected]>
> > ---
> > drivers/net/wimax/i2400m/fw.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wimax/i2400m/fw.c b/drivers/net/wimax/i2400m/fw.c
> > index e9fc168..6b36f6d 100644
> > --- a/drivers/net/wimax/i2400m/fw.c
> > +++ b/drivers/net/wimax/i2400m/fw.c
> > @@ -342,6 +342,7 @@ int i2400m_barker_db_init(const char *_options)
> > "a 32-bit number\n",
> > __func__, token);
> > result = -EINVAL;
> > + kfree(options_orig);
> > goto error_parse;
> > }
> > if (barker == 0) {
> > @@ -350,8 +351,10 @@ int i2400m_barker_db_init(const char *_options)
> > continue;
> > }
> > result = i2400m_barker_db_add(barker);
> > - if (result < 0)
> > + if (result < 0) {
> > + kfree(options_orig);
> > goto error_add;
>
> I know that you didn't add this error_add label, but it seems like the
> incorrect goto label. Although looking at the caller indicates an add
> failed, this label is used prior to and after the memory leak you are
> trying to fix. It might be better to change this label to something
> like error_parse_add and move the kfree to the unwinding. If a new
> label is used, it becomes more clear as to what is being undone and
> there aren't two jumps into an unwind from two very different stages of
> the function. Adding a new label also has the benefit of moving the
> kfree to the unwind of error_parse.

Thanks for your suggestion! I will rework the patch.

Wenwen