On Thu, 8 Dec 2022 at 23:35, Ard Biesheuvel <a...@kernel.org> wrote: > > On Thu, 8 Dec 2022 at 22:57, Kinney, Michael D > <michael.d.kin...@intel.com> wrote: > > > > 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'm surprised that this is the only offending line, and I suppose that > is good news. > > But as you said, the shell protocol is used much more widely, and > existing callers may rely on the destructive behavior. > > At the very least, we should only perform the SetDevicePathEndNode() > call if the node in question is not already an > end-of-entire-devicepath node, as the update is pointless in that > case, but will still trigger a fault if the memory is read-only. > > But it really depends on whether any callers might exist that expect a > multi-instance devicepath to be split up. > > > 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. > > > > Agreed. > > 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. > > > > OK, so what is the way forward here? >
I sent this before I noticed your other reply. So let's go with your fix to preserve the existing behavior without triggering the fault. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97153): https://edk2.groups.io/g/devel/message/97153 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] -=-=-=-=-=-=-=-=-=-=-=-
