lkml.org 
[lkml]   [2016]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure
From
Date
Markus,

On Mon, 2016-09-26 at 17:43 +0200, SF Markus Elfring wrote:
> Memory was not released (as it would be expected) when one call
> of further resource reservations failed.

This was the only thing in this series that triggered more than a, very
uninspired, "meh" on first read.

> * Split a condition check for memory allocation failures so that
>   each pointer from these function calls will be checked immediately.
>
>   See also background information:
>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>   Link: https://cwe.mitre.org/data/definitions/754.html

A quick scan of that link suggests we can do without the above
"background information" in the commit explanation.

> * Adjust jump targets according to the Linux coding style convention.

Another "meh".

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/isdn/gigaset/common.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/isdn/gigaset/common.c
> b/drivers/isdn/gigaset/common.c
> index c05a2a4..2e9382f 100644
> --- a/drivers/isdn/gigaset/common.c
> +++ b/drivers/isdn/gigaset/common.c
> @@ -710,10 +710,13 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
>   cs->mode = M_UNKNOWN;
>   cs->mstate = MS_UNINITIALIZED;
> cs->bcs = kmalloc_array(channels, sizeof(*cs->bcs), GFP_KERNEL);
> + if (!cs->bcs)
> + goto report_failure;
> +
>   cs->inbuf = kmalloc(sizeof(*cs->inbuf), GFP_KERNEL);
> - if (!cs->bcs || !cs->inbuf) {
> - goto error;
> - }
> + if (!cs->inbuf)
> + goto free_bcs;
> +
>   ++cs->cs_init;
>  
>   gig_dbg(DEBUG_INIT, "setting up at_state");
> @@ -737,14 +740,14 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
>   gig_dbg(DEBUG_INIT, "setting up iif");
>   if (gigaset_isdn_regdev(cs, modulename) < 0) {
>   pr_err("error registering ISDN device\n");
> - goto error;
> + goto free_bcs;
>   }
>  
>   make_valid(cs, VALID_ID);
>   ++cs->cs_init;
>   gig_dbg(DEBUG_INIT, "setting up hw");
>   if (cs->ops->initcshw(cs) < 0)
> - goto error;
> + goto free_bcs;
>  
>   ++cs->cs_init;
>  
> @@ -759,7 +762,7 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
>   gig_dbg(DEBUG_INIT, "setting up bcs[%d]", i);
>   if (gigaset_initbcs(cs->bcs + i, cs, i) < 0) {
> pr_err("could not allocate channel %d data\n", i);
> - goto error;
> + goto free_bcs;
>   }
>   }
>  
> @@ -772,8 +775,9 @@ struct cardstate *gigaset_initcs(struct gigaset_driver *drv, int channels,
>  
>   gig_dbg(DEBUG_INIT, "cs initialized");
>   return cs;
> -
> -error:
> +free_bcs:
> + kfree(cs->bcs);
> +report_failure:
>   gig_dbg(DEBUG_INIT, "failed");
>   gigaset_freecs(cs);

gigaset_freecs() is not a function I look at for the fun of it. But
still, in it we find:

case 0: /* error in basic setup */
[...]
kfree(cs->inbuf);
kfree(cs->bcs);

As far as I can tell we will call those two kfree()'s if we jump to
"error". So, contrary to your analysis, I don't think we leak cs->bcs.

>   return NULL;


Paul Bolle

\
 
 \ /
  Last update: 2016-09-26 23:14    [W:1.549 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site