Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752719AbcDZLqw (ORCPT ); Tue, 26 Apr 2016 07:46:52 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:40672 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751027AbcDZLqv (ORCPT ); Tue, 26 Apr 2016 07:46:51 -0400 Date: Tue, 26 Apr 2016 14:46:43 +0300 From: Dan Carpenter To: Claudiu Beznea Cc: gregkh@linuxfoundation.org, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] Staging: wlan-ng: memory allocated inside mkimage() is not freed if subsequent calls fails. Message-ID: <20160426100032.GL4298@mwanda> References: <1461516013-4383-1-git-send-email-claudiu.beznea@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1461516013-4383-1-git-send-email-claudiu.beznea@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2541 Lines: 78 This patch makes the code better and it's an improvement so I'm fine with merging it as-is. On Sun, Apr 24, 2016 at 07:40:13PM +0300, Claudiu Beznea wrote: > This patch frees memory allocated inside mkimage() in case mkimage() > or any other subsequent calls inside prism2_fwapply() from prism2fw.c > file fails. To fix this I introduces goto labels where the free > operation is done in case some operations fails. After the introduction > of goto labels has been done, in order to use the same return path, > "return x" instuctions were replaced with "goto" instuctions. > > Signed-off-by: Claudiu Beznea > --- > drivers/staging/wlan-ng/prism2fw.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/drivers/staging/wlan-ng/prism2fw.c b/drivers/staging/wlan-ng/prism2fw.c > index 8564d9e..56bffd9 100644 > --- a/drivers/staging/wlan-ng/prism2fw.c > +++ b/drivers/staging/wlan-ng/prism2fw.c > @@ -278,7 +278,8 @@ static int prism2_fwapply(const struct ihex_binrec *rfptr, > /* Build the PDA we're going to use. */ > if (read_cardpda(&pda, wlandev)) { > netdev_err(wlandev->netdev, "load_cardpda failed, exiting.\n"); > - return 1; > + result = 1; > + goto out; It's better to do direct returns instead of misleading gotos that don't do anything. "Future proofing" is a waste of time and introduces more bugs than it prevents. Just write for the present. Present proof the code. > } > > /* read the card's PRI-SUP */ > @@ -315,55 +316,58 @@ static int prism2_fwapply(const struct ihex_binrec *rfptr, > if (result) { > netdev_err(wlandev->netdev, > "Failed to read the data exiting.\n"); > - return 1; > + goto out; After this should be goto free_srecs not goto out. I don't know that it matters... > } > > result = validate_identity(); > - > if (result) { > netdev_err(wlandev->netdev, "Incompatible firmware image.\n"); > - return 1; > + goto out; > } > > if (startaddr == 0x00000000) { > netdev_err(wlandev->netdev, > "Can't RAM download a Flash image!\n"); > - return 1; > + result = 1; > + goto out; > } > > /* Make the image chunks */ > result = mkimage(fchunk, &nfchunks); > if (result) { > netdev_err(wlandev->netdev, "Failed to make image chunk.\n"); > - return 1; > + goto free_chunks; This works, but really mkimage() should clean up it's own allocations on failure. Otherwise it is a layering violation. The rest of the patch is fine. regards, dan carpenter