On Thu, 8 Dec 2022 at 20:20, Kinney, Michael D <michael.d.kin...@intel.com> wrote: > > Hi Ard, > > Much of this code has not been updated since initially added in 2010. > > Looks like a bug to me that has been there the whole time. > > I agree it is a behavior change in the implementation. But unless > new code use of this API looks at the implementation, they would > not know it is destructive and they need to make a copy. This > API is available to external shell apps that use the shell protocol. >
Well, not entirely. The function takes EFI_DEVICE_PATH_PROTOCOL** not CONST EFI_DEVICE_PATH_PROTOCOL**, and so one might argue that the underlying object is modifiable by the callee. And similarly, that shell code should not grab a EFI device path protocol pointer from the database and pass it to a function that does not use a CONST qualified EFI_DEVICE_PATH_PROTOCOL pointer to accept the argument. > We should get the shell owners to evaluate removing the destructive > behavior. > I suppose changing the prototypes is out of the question, as doing so would require a new version of the shell protocol? > > -----Original Message----- > > From: Ard Biesheuvel <a...@kernel.org> > > Sent: Thursday, December 8, 2022 10:45 AM > > To: Kinney, Michael D <michael.d.kin...@intel.com> > > Cc: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; Gao, Zhichao > > <zhichao....@intel.com> > > Subject: Re: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed > > device path protocols > > > > On Thu, 8 Dec 2022 at 19:28, Kinney, Michael D > > <michael.d.kin...@intel.com> wrote: > > > > > > Hi Ard, > > > > > > From this description, it does not look like it should be modifying the > > > contents of the device path. Just point to the device path end node that > > > follows the match found. > > > > > > /** > > > Gets the mapping that most closely matches the device path. > > > > > > This function gets the mapping which corresponds to the device path > > > *DevicePath. If > > > there is no exact match, then the mapping which most closely matches > > > *DevicePath > > > is returned, and *DevicePath is updated to point to the remaining > > > portion of the > > > device path. If there is an exact match, the mapping is returned and > > > *DevicePath > > > points to the end-of-device-path node. > > > > > > @param DevicePath On entry, points to a device path > > > pointer. On > > > exit, updates the pointer to point to the > > > portion of the device path after the > > > mapping. > > > > > > @retval NULL No mapping was found. > > > @return !=NULL Pointer to NULL-terminated mapping. The > > > buffer > > > is callee allocated and should be freed > > > by the caller. > > > **/ > > > CONST CHAR16 * > > > EFIAPI > > > EfiShellGetMapFromDevicePath ( > > > IN OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath > > > ); > > > > > > I see this API used in many places, and it looks like it would be > > > destructive to any multi-instance device path. Multi-instance > > > device paths are typically used for consoles, so we may not have > > > noticed this destructive behavior with file system mapping paths. > > > > > > Did you try removing the call to SetDevicePathEndNode (*DevicePath); ? > > > > > > > No, but that would be a functional change visible to all users of the > > current API. > > > > And note that the calling code already has 'DevicePathCopy' variables, > > it just doesn't bother using them, so the intent is clearly to pass a > > copy, not the actual device path. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97147): https://edk2.groups.io/g/devel/message/97147 Mute This Topic: https://groups.io/mt/95518373/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
