Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753733Ab0LaPAq (ORCPT ); Fri, 31 Dec 2010 10:00:46 -0500 Received: from swampdragon.chaosbits.net ([90.184.90.115]:14876 "EHLO swampdragon.chaosbits.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752161Ab0LaPAp (ORCPT ); Fri, 31 Dec 2010 10:00:45 -0500 Date: Fri, 31 Dec 2010 15:51:41 +0100 (CET) From: Jesper Juhl To: Malcolm Priestley cc: linux-kernel@vger.kernel.org, Mauro Carvalho Chehab Subject: Re: [PATVH] media, dvb, IX2505V: Remember to free allocated memory in failure path (ix2505v_attach()). In-Reply-To: <1293758374.10326.7.camel@tvboxspy> Message-ID: References: <1293758374.10326.7.camel@tvboxspy> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2946 Lines: 104 On Fri, 31 Dec 2010, Malcolm Priestley wrote: > On Fri, 2010-12-31 at 00:11 +0100, Jesper Juhl wrote: > > Hi, > > > > We may leak the storage allocated to 'state' in > > drivers/media/dvb/frontends/ix2505v.c::ix2505v_attach() on error. > > This patch makes sure we free the allocated memory in the failure case. > > > > > > Signed-off-by: Jesper Juhl > > --- > > ix2505v.c | 1 + > > 1 file changed, 1 insertion(+) > > > > Compile tested only. > > > > diff --git a/drivers/media/dvb/frontends/ix2505v.c b/drivers/media/dvb/frontends/ix2505v.c > > index 55f2eba..fcb173d 100644 > > --- a/drivers/media/dvb/frontends/ix2505v.c > > +++ b/drivers/media/dvb/frontends/ix2505v.c > > @@ -293,6 +293,7 @@ struct dvb_frontend *ix2505v_attach(struct dvb_frontend *fe, > > ret = ix2505v_read_status_reg(state); > > > > if (ret & 0x80) { > > + kfree(state); > > deb_i2c("%s: No IX2505V found\n", __func__); > > goto error; > > } > > > Memory is freed in... > > error: > ix2505v_release(fe); > return NULL; > > via... > > static int ix2505v_release(struct dvb_frontend *fe) > { > struct ix2505v_state *state = fe->tuner_priv; > > fe->tuner_priv = NULL; > kfree(state); > > return 0; > } > Except that 'state' has not been assigned to fe->tuner_priv at this point, so ix2505v_release() cannot free the memory that was just allocated with kzalloc(). state is a local variable: struct ix2505v_state *state = NULL; ... we allocate memory and assign it to 'state' here: state = kzalloc(sizeof(struct ix2505v_state), GFP_KERNEL); if (NULL == state) return NULL; state->config = config; state->i2c = i2c; here 'state' is used, but not in a way that saves it anywhere: if (state->config->tuner_write_only) { if (fe->ops.i2c_gate_ctrl) fe->ops.i2c_gate_ctrl(fe, 1); this function call involves 'state' but it does not save it anywhere either: ret = ix2505v_read_status_reg(state); if (ret & 0x80) { deb_i2c("%s: No IX2505V found\n", __func__); so when we jump to error here 'state' still exists only as the local variable, it has not been assigned to anything else. goto error; } ... error: there is no way this function call can free 'state' on this path since it has not been assigned to fe->tuner_priv. ix2505v_release(fe); The local variable state goes out of scope here and leaks the memory it points to: return NULL; } Am I missing something? -- Jesper Juhl http://www.chaosbits.net/ Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/