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 -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
