From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c0c::244; helo=mail-wr0-x244.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x244.google.com (mail-wr0-x244.google.com [IPv6:2a00:1450:400c:c0c::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 955672034CF7D for ; Tue, 7 Nov 2017 09:51:03 -0800 (PST) Received: by mail-wr0-x244.google.com with SMTP id y9so48482wrb.2 for ; Tue, 07 Nov 2017 09:55:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=vKuSd2uSdBm6mU3w2P58Y4xVpOVEIxSAFjH5cT2m/k0=; b=WMHAjLZYP+5Q+3Kmj3zP5Lx4BpN+n3idtsxsXpEvRhALGJzQTA0Jv1sYQhSft/O7eq MfwsEDqmcQzt8wITmjrthw7lpjA+jFf6EheTysmx8KSPyU0Oe6b6gtUx9SEUhKyZ605p z1DzICZvWBoVn2rnXom3SubIB/eyXc23Xd9o4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=vKuSd2uSdBm6mU3w2P58Y4xVpOVEIxSAFjH5cT2m/k0=; b=FTZ9x86xh1dbjKBDoDRC8qOniu24YQgpNUqoZgvojKlDut9UXrOZUPJH6yBLUWANei rL6tKzhfEGuSw7g084AD4bKmQ05rsPZ6Y3Uwcb9roQskVUoQC/P3SKLpVedJ/7aH43Eo WoSEDi4xlP71IyxeJPi8Q8zNweb1OwbdFxthWIR+9CwgrNZ7JNcxbObcfUGh2V76w26J Eb7kmsbV8AvXQDZi8EV0yk5HVT3PvDOL4w+RJOjGBRtQOvvNtL54m9vzCDjXOUQN3nbU olk6A08f24FOlboByAWiZ0Pz2+7PFVlzyFUPETLIFEdsYZ+6JSw/xYXfxN4+I4Iws9xZ Hf+w== X-Gm-Message-State: AJaThX6TYPGTqBmqHBj0I8ozchLOyttTAYa6EAEJ+8Idi449JpPixQjq kkqovgbh9hjifyFG6qc1OT7U5Q== X-Google-Smtp-Source: ABhQp+QtVXdPfwsbOYTB9k4TmMhOpzGbLD9676u1515d0SSsT/Fj+izNI60vNHJvHo/DwjoYIvz5ng== X-Received: by 10.223.187.3 with SMTP id r3mr5025971wrg.34.1510077301896; Tue, 07 Nov 2017 09:55:01 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id t43sm1173743wrc.74.2017.11.07.09.55.00 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 07 Nov 2017 09:55:00 -0800 (PST) Date: Tue, 7 Nov 2017 17:54:59 +0000 From: Leif Lindholm To: Vabhav Cc: edk2-devel@lists.01.org, ruiyu.ni@intel.com, jaben.carsey@intel.com, ard.biesheuvel@linaro.org, siyuan.fu@intel.com, ting.ye@intel.com, Udit Kumar Message-ID: <20171107175458.f4pgne4rkri7lzch@bivouac.eciton.net> References: <1509719192-16582-1-git-send-email-vabhav.sharma@nxp.com> MIME-Version: 1.0 In-Reply-To: <1509719192-16582-1-git-send-email-vabhav.sharma@nxp.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH] Tftp assert fix for openfile failure case X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Nov 2017 17:51:04 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Nov 03, 2017 at 07:56:32PM +0530, Vabhav wrote: > Issue: > when file open is failed, assert was seen due to freeing 0 size page > > Reason: > DataSize is remain zero if error is reported in ShellOpenFileByName > > Fix: > Update DataSize as soon as FileSize is available > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Udit Kumar > Signed-off-by: Vabhav > --- > ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > index fbde3bf..6425fc5 100755 > --- a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > +++ b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > @@ -509,6 +509,7 @@ ShellCommandRunTftp ( > ); > goto NextHandle; Wow, a goto in a foor loop in a 320-line function. What could possibly go wrong? > } > + DataSize = FileSize; > > Status = DownloadFile (Mtftp4, RemoteFilePath, AsciiRemoteFilePath, FileSize, BlockSize, &Data); > if (EFI_ERROR (Status)) { > @@ -539,7 +540,6 @@ ShellCommandRunTftp ( > goto NextHandle; > } > > - DataSize = FileSize; > Status = ShellWriteFile (FileHandle, &FileSize, Data); > if (!EFI_ERROR (Status)) { > ShellStatus = SHELL_SUCCESS; > -- > 1.9.1 So, a wider question: This shell command was introduced in the heyday of "let's reimplement U-Boot in the EDK2 tree". Mainly, from my impression, it seems to be used in order that people don't need to learn how boot managers and device paths work. Am I being too harsh? Are there practical uses for this? Or should we delete it from the tree? If the code is to be kept, I think (from a quick glance) that I would also like to see *Data = NULL in the error path of DownloadFile(). / Leif