Re: [PATCH 4/5] ISDN-Gigaset: Release memory in gigaset_initcs() after an allocation failure

From: Paul Bolle
Date: Mon Sep 26 2016 - 17:14:09 EST


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@xxxxxxxxxxxxxxxxxxxxx>
> ---
> Â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