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

[Paul Bolle]

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