From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 794FC2035D6B1 for ; Thu, 26 Apr 2018 03:20:51 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 22D088D685; Thu, 26 Apr 2018 10:20:50 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-36.rdu2.redhat.com [10.10.120.36]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4137A215CDCB; Thu, 26 Apr 2018 10:20:48 +0000 (UTC) To: Haojian Zhuang Cc: "edk2-devel@lists.01.org" , "Leif Lindholm (Linaro address)" , Ard Biesheuvel References: <1524632376-23501-1-git-send-email-haojian.zhuang@linaro.org> <0a629a97-9b8b-08a5-c04f-5bba40a1a5ff@redhat.com> From: Laszlo Ersek Message-ID: <32e983cd-ec8e-43bf-4db8-93949a6b0894@redhat.com> Date: Thu, 26 Apr 2018 12:20:48 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Thu, 26 Apr 2018 10:20:50 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Thu, 26 Apr 2018 10:20:50 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH edk2-platforms v5 0/2] add platform boot options X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 26 Apr 2018 10:20:51 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/26/18 05:12, Haojian Zhuang wrote: > On 26 April 2018 at 00:10, Laszlo Ersek wrote: >> (4) In both patches, in both of the CreatePlatformBootOptionFromPath() >> and CreatePlatformBootOptionFromGuid() helper functions, the >> "DevicePath" variable is leaked when >> EfiBootManagerInitializeLoadOption() fails. >> >> "DevicePath" should be freed unconditionally at that point, in both >> patches and in both helper functions (4 instances in total). Simply >> eliminate the following: >> >> if (EFI_ERROR (Status)) { >> return Status; >> } >> >> and you will be left with: >> >> Status = EfiBootManagerInitializeLoadOption ( >> ... >> ); >> FreePool (DevicePath); >> return Status; >> >> I believe I need not separately review this update either. >> > > For this one, I have a different opinion. > > EFI_STATUS > EFIAPI > EfiBootManagerInitializeLoadOption ( > ... > ) > { > if ((Option == NULL) || (Description == NULL) || (FilePath == NULL)) { > return EFI_INVALID_PARAMETER; > } > > if (((OptionalData != NULL) && (OptionalDataSize == 0)) || > ((OptionalData == NULL) && (OptionalDataSize != 0))) { > return EFI_INVALID_PARAMETER; > } > if ((UINT32) OptionType >= LoadOptionTypeMax) { > return EFI_INVALID_PARAMETER; > } > > ZeroMem (Option, sizeof (EFI_BOOT_MANAGER_LOAD_OPTION)); > Option->OptionNumber = OptionNumber; > Option->OptionType = OptionType; > Option->Attributes = Attributes; > Option->Description = AllocateCopyPool (StrSize > (Description), Description); > Option->FilePath = DuplicateDevicePath (FilePath); > if (OptionalData != NULL) { > Option->OptionalData = AllocateCopyPool (OptionalDataSize, > OptionalData); > Option->OptionalDataSize = OptionalDataSize; > } > > return EFI_SUCCESS; > } > > We can find that no memory is allocated to "DevicePath" if it returns > with error. > > So we shouldn't free memory on "DevicePath" if "Status" is error code. I disagree. We have: CreatePlatformBootOptionFromPath() DevicePath = ConvertTextToDevicePath(...) #1 EfiBootManagerInitializeLoadOption(... DevicePath ...) DuplicateDevicePath(FilePath=DevicePath) #2 The ConvertTextToDevicePath() function returns the binary representation of the device path in a dynamically allocated area. That is dynamic object #1, tracked by the "DevicePath" variable. Then, EfiBootManagerInitializeLoadOption() creates a deep copy, by calling the DuplicateDevicePath() function. That creates dynamic object #2, tracked by Option->FilePath. - If EfiBootManagerInitializeLoadOption() returns with failure, then dynamic object #2 does not exist, and we no longer need dynamic object #1, so we have to free dynamic object #1. - If EfiBootManagerInitializeLoadOption() returns with success, then dynamic object #2 exists, and is correctly tracked by Option->FilePath. We no longer need dynamic object #1, so we have to free it. In other words, regardless of the return status of EfiBootManagerInitializeLoadOption(), we must release dynamic object #1. We need dynamic object #1 only temporarily, so we can call EfiBootManagerInitializeLoadOption(), and let it make a copy. The same applies to CreatePlatformBootOptionFromGuid(), except replace ConvertTextToDevicePath() with AppendDevicePathNode(): CreatePlatformBootOptionFromGuid() DevicePath = AppendDevicePathNode(...) #1 EfiBootManagerInitializeLoadOption (... DevicePath ...) DuplicateDevicePath(FilePath=DevicePath) #2 Again, AppendDevicePathNode() produces dynamic object #1, similarly to ConvertTextToDevicePath(). Thanks Laszlo