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

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

Ard,

Thank you for the correction.

If we add that CONST, then the ShellPkg build breaks with an error

c:\work\github\tianocore\edk2\ShellPkg\Application\Shell\ShellProtocol.c(297): error C2220: the following warning is treated as an error
c:\work\github\tianocore\edk2\ShellPkg\Application\Shell\ShellProtocol.c(297): warning C4090: 'function': different 'const' qualifiers

Which is exactly the line we want to remove to prevent the destructive behavior.

    SetDevicePathEndNode (*DevicePath);

If I comment out that line, the ShellPkg build completes with no errors.

I agree that it would be better to update the prototype and get help
from the compiler to find incorrect implementations.  Even though
CONST is not in the prototype, from reading the description of the API
it does not state that the contents are modified, so I think the 
intent was no modifications.

Your suggested change is safe, but it is incomplete because there
are additional calls through the protocol that are not covered
by your patch.  We also do not know how many places this API
is used in downstream projects.  This side effect of a write to
a read-only page and potential corruption of a multi-instance
device path looks like a bug to me and we should fix the root
cause and not fix just some of the callers.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Thursday, December 8, 2022 1:40 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: 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 22:15, Michael D Kinney
> <michael.d.kinney@intel.com> wrote:
> >
> > Hi Ard,
> >
> > There is a difference between returning a pointer to a device path
> > and modifying the device path contents.
> >
> > If you add CONST to the argument, then an updated pointer to a device
> > path can not be returned.
> >
> 
> No, this is incorrect.
> 
> The function takes a pointer (1) to a pointer(2)  to a device path protocol
> 
> EFI_DEVICE_PATH_PROTOCOL **
> 
> So the function can dereference pointer 1 and modify pointer 2
> *unless* it is marked as CONST, i.e.
> 
> EFI_DEVICE_PATH_PROTOCOL * CONST *
> 
> in which case the pointer is not modifiable, but it is permitted to
> dereference that pointer to modify the underlying object.
> 
> I am arguing that the prototype should be
> 
> EFI_DEVICE_PATH_PROTOCOL CONST **
> 
> (which is the same as putting the CONST at the beginning)
> 
> where the caller's pointer can be advanced by the callee via the
> pointer-to-pointer. But that would still not permit the object to be
> modified.
> 
> > The API clear describes returning an updated device path pointer, so
> > the API is declared correctly without CONST.
> >
> 
> The pointer may be updated but not the object. It really comes down to
> the difference between
> 
> CONST EFI_DEVICE_PATH_PROTOCOL **
> EFI_DEVICE_PATH_PROTOCOL CONST **
> EFI_DEVICE_PATH_PROTOCOL * CONST *
> EFI_DEVICE_PATH_PROTOCOL **CONST
> 
> (where the first two mean the same thing0
> 
> > The API does not state that the contents of the device path are modified.
> >
> > An API that uses CONST EFI_DEVICE_PATH* would indicate that the API
> > should not modify the contents of the device path.  For example:
> >
> > /**
> >   Returns the size of a device path in bytes.
> >
> >   This function returns the size, in bytes, of the device path data structure
> >   specified by DevicePath including the end of device path node.
> >   If DevicePath is NULL or invalid, then 0 is returned.
> >
> >   @param  DevicePath  A pointer to a device path data structure.
> >
> >   @retval 0           If DevicePath is NULL or invalid.
> >   @retval Others      The size of a device path in bytes.
> >
> > **/
> > UINTN
> > EFIAPI
> > GetDevicePathSize (
> >   IN CONST EFI_DEVICE_PATH_PROTOCOL  *DevicePath
> >   );
> >
> 
> Yes, but this one is a pointer, not a pointer-to-pointer. Big difference.
> 
> 
> 
>