Re: [edk2-devel] [PATCH] ShellPkg: Avoid corrupting installed device path protocols

["Michael D Kinney" <michael.d.kinney@intel.com>]

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.

We should get the shell owners to evaluate removing the destructive
behavior.

Mike

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Thursday, December 8, 2022 10:45 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@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.kinney@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.