Good day, Thank you, Laszlo, for your ambition to introduce stricter code style enforcements. Sorry to "hijack" the actual topic (I did not CC anyone on purpose, as this is mostly a separate topic and I'd like a quick comment first), but this seems like a good occasion to mention another few bad practices edk2 has been following. Mainly, I'd like to call *some* attention to quality problems in the code base while this has some traction, and cause a discussion on whether and how those are to be approached.
Thank you for your time. Regards, Marvin "inadequate type punning": e.g. https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdeModulePkg/Bus/Usb/UsbBusPei/HubPeim.c#L446 This is mostly about the infamous "Strict Aliasing" rule, which is basically: "An object shall have its stored value accessed only by an lvalue expression that has one of the following types: — a type compatible with the effective type of the object, — a qualifed version of a type compatible with the effective type of the object, — a type that is the signed or unsigned type corresponding to the effective type of the object, — a type that is the signed or unsigned type corresponding to a qualifed version of the effective type of the object, — an aggregate or union type that includes one of the aforementioned types among its members (including, recursively, a member of a subaggregate or contained union), or — a character type." C18 (ISO/IEC 9899:2018), 6.5.7 (exists, though has been updated, since C90) Currently optimisations based on this are disabled. This is a bit nasty to work around if *seriously* needed when sticking to C90, I can only think of memcpy right now. However, even though there are compilers that do not fully support C99 (ahem, Microsoft :) ), type-punning by unions should be supported by them all, and has been legal as of C99, where the following part has been dropped from the standard: "With one exception, if a member of a union object is accessed after a value has been stored in a different member of the object, the behavior is implementation-defined." C90 (ISO/IEC 9899:1990), 6.3.2.3 "pointer unions": e.g. https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Include/IndustryStandard/SmBios.h#L2592 While the idea behind them is certainly style preference, using a union of pointers prevents two important things over a union of structs. 1) CONST declaration: When defining a variable of a union type containing pointers as CONST, speaking of its members, they are all going to be CONST pointers to arbitrary memory and not arbitrary pointers to CONST memory. With a union of structs, you can have either as required (e.g. CONST UNION_TYPE *union, or UNION_TYPE *COST union). 2) Well-defined header inspection: "if a union contains several structures that share a common initial sequence [...], it is permitted to inspect the common initial part of any of them anywhere that a declaration of the completed type of the union is visible" C18 (ISO/IEC 9899:2018), 6.5.2.3.6 (exists since at least C90) This guarantee can be used to inspect the type defined in a common header (e.g. SMBIOS_STRUCTURE) and the process the type-specific data by accessing the appropiate member (e.g. SMBIOS_TABLE_TYPE0) legally. Plain casts and "pointer union" accesses are illegal as per the "inadequate type punning" point above. "casting away CONST": e.g. https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Library/BaseIoLibIntrinsic/IoLibMmioBuffer.c#L236 This should be obvious as Undefined Behaviour because memory previously guaranteed to be read-only is returned as a pointer to memory that allows writing, but for easier lookup, here's the related rule: "the left operand has atomic, qualifed, or unqualifed pointer type, and [...] the type pointed to by the left has all the qualifers of the type pointed to by the right" C18 (ISO/IEC 9899:2018), 6.5.16.1 (exists since at least C90) "structs with trailing 1-length array" e.g. https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Include/Guid/FileInfo.h#L51 This is undefined as per: "The behavior is undefned in the following circumstances: [...] — Addition or subtraction of a pointer into, or just beyond, an array object and an integer type produces a result that does not point into, or just beyond, the same array object (6.5.6). — Addition or subtraction of a pointer into, or just beyond, an array object and an integer type produces a result that points just beyond the array object and is used as the operand of a unary * operator that is evaluated (6.5.6). — Pointers that do not point into, or just beyond, the same array object are subtracted (6.5.6)." C18 (ISO/IEC 9899:2018), J.2 (exists since at least C90) Same as above, while not all compilers fully support C99, flexible arrays should be support by all reasonably new compilers and allow a legal declaration and usage where this hack is in place. At worst, a macro could be provided to declare a [1] vs a [] array on demand and a requirement be introduced to have a "SIZE_OF_" macro for each such struct, but my personal preference would be to just enforce flexible arrays. "Missing security checks for external data": e.g. https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L943 The given example misses an alignment verification of the resulting pointer (which technically has to be verified *before* casting), as documented here: "The behavior is undefined in the following circumstances: [...] — Conversion between two pointer types produces a result that is incorrectly aligned (6.3.2.3)." C18 (ISO/IEC 9899:2018), J.2 (exists since at least C90) There are more such issues throughout the codebase, including missing overflow and (or flawed, see before) bounds checks, however I cannot find such quickly. "signed int BIT definitions": e.g. https://github.com/tianocore/edk2/blob/fcdedafd97c8f18c33a63d26b954e5dbaee81a2b/MdePkg/Include/Base.h#L348 Fixing this would be prone to regressions, but I'd like to add it for tracking purposes. Related discussion can be found down the chain here: https://lists.01.org/pipermail/edk2-devel/2018-February/021919.html Am 17.09.2019 um 21:49 schrieb Laszlo Ersek: > Repository: https://github.com/lersek/edk2.git > Branch: voidptr > > The UEFI / PI / Shell specifications define a number of standard types > as pointers to VOID. This is arguably a design mistake; those types > should have been pointers to distinct incomplete union or structure > types. Here's why: > > Roughly paraphrasing the constraints from ISO C99 "6.5.16.1 Simple > assignment" and "6.5.4 Cast operators", any pointer-to-object type > converts implicitly to, and from, pointer-to-void, provided const / > volatile qualifications are not relaxed. Such implicit conversions > prevent compilers from catching at least the following two kinds of > coding mistakes: > > - mixing up one type with another (for example, EFI_HANDLE with > EFI_EVENT), > > - getting the depth of indirection wrong (for example, mixing up > (EFI_HANDLE*) with EFI_HANDLE). > > This series first separates these standard types from each other, in the > first patch, which is *not* being proposed for merging. This unmasks a > number of warts (semantic issues, or actual bugs) in the source code, in > the form of build breakages. The rest of the series works through those > breakages, cleaning and fixing the code. > > Every DSC file in the edk2 tree was built for at least one of the NOOPT, > DEBUG, RELEASE targets (NOOPT being preferred), with the GCC48 toolchain > (for IA32 / X64) and the GCC5 toolchain (for ARM / AARCH64). Of course, > the build arches were restricted to the SUPPORTED_ARCHITECTURES stated > in the individual DSC files. > > There were two exceptions to the above rule: DynamicTablesPkg was only > build-tested with AARCH64 (despite its SUPPORTED_ARCHITECTURES), given > that 32-bit ARM has no ACPI bindings. StandaloneMmPkg too was only > build-tested with AARCH64; it doesn't actually support IA32/X64 yet. > > Regarding boot & runtime tests, ArmVirtQemu on AARCH64 was tested with > booting to the OS (RHEL7). Furthermore, I exercised OVMF with my usual > boot and S3 tests, covering IA32, IA32X64, and X64. Finally, some other > individual tests (noted per patch) were done with OVMF. > > Cc: Achin Gupta <achin.gu...@arm.com> > Cc: Andrew Fish <af...@apple.com> > Cc: Anthony Perard <anthony.per...@citrix.com> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > Cc: Benjamin You <benjamin....@intel.com> > Cc: Chao Zhang <chao.b.zh...@intel.com> > Cc: Dandan Bi <dandan...@intel.com> > Cc: David Woodhouse <dw...@infradead.org> > Cc: Eric Dong <eric.d...@intel.com> > Cc: Guo Dong <guo.d...@intel.com> > Cc: Hao A Wu <hao.a...@intel.com> > Cc: Jaben Carsey <jaben.car...@intel.com> > Cc: Jian J Wang <jian.j.w...@intel.com> > Cc: Jian Wang <jian.j.w...@intel.com> > Cc: Jiaxin Wu <jiaxin...@intel.com> > Cc: Jiewen Yao <jiewen....@intel.com> > Cc: Jordan Justen <jordan.l.jus...@intel.com> > Cc: Julien Grall <julien.gr...@arm.com> > Cc: Leif Lindholm <leif.lindh...@linaro.org> > Cc: Liming Gao <liming....@intel.com> > Cc: Maurice Ma <maurice...@intel.com> > Cc: Michael D Kinney <michael.d.kin...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Cc: Siyuan Fu <siyuan...@intel.com> > Cc: Supreeth Venkatesh <supreeth.venkat...@arm.com> > Cc: Zhichao Gao <zhichao....@intel.com> > > Thanks > Laszlo > > Laszlo Ersek (35): > DO NOT APPLY: edk2: turn standard handle types into pointers to > non-VOID > EmbeddedPkg: add missing EFIAPI calling convention specifiers > EmbeddedPkg/AndroidFastbootTransportTcpDxe: fix DestroyChild() call > EmbeddedPkg/Universal/MmcDxe: "fix" CloseProtocol() call in > BindingStop() > EmulatorPkg/DxeTimerLib: drop superfluous cast > EmulatorPkg: stop abusing EFI_HANDLE for keystroke notify registration > MdeModulePkg: fix cast in GetModuleInfoFromHandle() calls > MdeModulePkg/UefiHiiLib: stop using EFI_HANDLE in place of > EFI_HII_HANDLE > MdeModulePkg: stop abusing EFI_EVENT for protocol notify registration > MdeModulePkg/PlatformVarCleanupLib: fix HiiConstructConfigHdr() call > MdeModulePkg: document workaround for EFI_RUNTIME_EVENT_ENTRY PI spec > bug > MdeModulePkg: stop abusing EFI_HANDLE for keystroke notify > registration > MdeModulePkg: PEI Core: clean up "AprioriFile" handling in > FindFileEx() > MdeModulePkg: fix UninstallMultipleProtocolInterfaces() calls > MdeModulePkg/PiSmmCore: make type punning consistent > MdeModulePkg/S3SaveState: cast Position for S3BootScriptLib explicitly > MdePkg/DxeServicesLib: remove bogus cast > NetworkPkg/DxeNetLib: fix type typo in NetLibGetMacAddress() > NetworkPkg: fix CloseProtocol & UninstallMultipleProtocolInterfaces > calls > NetworkPkg/Ip4Dxe: fix NetLibDestroyServiceChild() call > NetworkPkg/TcpDxe: fix SockFreeFoo() parameter list > OvmfPkg/XenBusDxe: fix UninstallMultipleProtocolInterfaces() call > OvmfPkg/VirtioNetDxe: fix SignalEvent() call > OvmfPkg/PlatformDxe: fix EFI_HII_HANDLE parameters of internal > functions > OvmfPkg/VideoDxe: document EFI_EDID_OVERRIDE_PROTOCOL.GetEdid() call > SecurityPkg: fix UninstallMultipleProtocolInterfaces() calls > SecurityPkg: stop abusing EFI_EVENT for protocol notify registration > ShellPkg/UefiShellDriver1CommandsLib: fix parameter list typo > ShellPkg: stop using EFI_HANDLE in place of EFI_HII_HANDLE > ShellPkg: stop taking EFI_HANDLE in place of SHELL_FILE_HANDLE > ShellPkg/UefiShellDebug1CommandsLib: fix ShellCloseFile() call > ShellPkg/UefiShellLib: clarify workaround for unfixable EdkShell bug > StandaloneMmPkg/Core: stop abusing EFI_HANDLE for FwVolHeader tracking > UefiPayloadPkg/BlSupportPei: fix MMCONFIG assignment from XSDT > UefiPayloadPkg/BlSupportDxe: fix ReserveResourceInGcd() calls > > EmbeddedPkg/Drivers/AndroidFastbootTransportTcpDxe/FastbootTransportTcp.c > | 2 +- > EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c > | 1 + > EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c > | 1 + > EmbeddedPkg/Drivers/SataSiI3132Dxe/SataSiI3132.h > | 32 ++++++-- > EmbeddedPkg/Drivers/SataSiI3132Dxe/SiI3132AtaPassThru.c > | 8 ++ > EmbeddedPkg/GdbStub/GdbStubInternal.h > | 9 +++ > EmbeddedPkg/MetronomeDxe/Metronome.c > | 1 + > EmbeddedPkg/Universal/MmcDxe/Mmc.c > | 5 +- > EmulatorPkg/EmuGopDxe/GopInput.c > | 4 +- > EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.c > | 2 +- > MdeModulePkg/Bus/I2c/I2cDxe/I2cBus.c > | 2 +- > MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.c > | 2 +- > MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > | 6 +- > MdeModulePkg/Bus/Pci/PciSioSerialDxe/Serial.c > | 2 +- > MdeModulePkg/Bus/Sd/EmmcDxe/EmmcDxe.c > | 2 +- > MdeModulePkg/Bus/Usb/UsbBusDxe/UsbEnumer.c > | 2 +- > MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassImpl.c > | 2 +- > MdeModulePkg/Core/Dxe/Event/Event.c > | 8 ++ > MdeModulePkg/Core/Dxe/Event/Event.h > | 2 +- > MdeModulePkg/Core/Dxe/Hand/Handle.h > | 2 +- > MdeModulePkg/Core/Pei/FwVol/FwVol.c > | 2 +- > MdeModulePkg/Core/Pei/FwVol/FwVol.h > | 2 +- > MdeModulePkg/Core/PiSmmCore/PiSmmCore.h > | 2 +- > MdeModulePkg/Core/PiSmmCore/Smi.c > | 8 +- > MdeModulePkg/Core/RuntimeDxe/Runtime.c > | 10 ++- > MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib.c > | 12 +-- > MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c > | 6 +- > MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib.c > | 8 +- > MdeModulePkg/Library/UefiHiiLib/HiiString.c > | 4 +- > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.h > | 2 +- > MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveState.c > | 4 +- > MdeModulePkg/Universal/Acpi/SmmS3SaveState/SmmS3SaveState.c > | 4 +- > MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c > | 2 +- > MdeModulePkg/Universal/DriverSampleDxe/DriverSample.c > | 4 +- > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.c > | 2 +- > MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.c > | 2 +- > MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h > | 2 +- > MdeModulePkg/Universal/LoadFileOnFv2/LoadFileOnFv2.c > | 2 +- > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c > | 2 +- > MdePkg/Include/Pi/PiPeiCis.h > | 6 +- > MdePkg/Include/Protocol/AcpiSystemDescriptionTable.h > | 3 +- > MdePkg/Include/Protocol/Bis.h > | 3 +- > MdePkg/Include/Protocol/Eap.h > | 3 +- > MdePkg/Include/Protocol/HiiFont.h > | 3 +- > MdePkg/Include/Protocol/MmMp.h > | 3 +- > MdePkg/Include/Protocol/S3SaveState.h > | 2 +- > MdePkg/Include/Protocol/Shell.h > | 3 +- > MdePkg/Include/Protocol/UserManager.h > | 9 ++- > MdePkg/Include/Uefi/UefiBaseType.h > | 6 +- > MdePkg/Include/Uefi/UefiInternalFormRepresentation.h > | 3 +- > MdePkg/Library/DxeServicesLib/DxeServicesLib.c > | 2 +- > NetworkPkg/DnsDxe/DnsDriver.c > | 4 +- > NetworkPkg/IScsiDxe/IScsiConfig.c > | 2 +- > NetworkPkg/Ip4Dxe/Ip4Driver.c > | 2 +- > NetworkPkg/Ip4Dxe/Ip4If.c > | 4 +- > NetworkPkg/Ip6Dxe/Ip6Driver.c > | 2 +- > NetworkPkg/Library/DxeNetLib/DxeNetLib.c > | 2 +- > NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c > | 2 +- > NetworkPkg/TcpDxe/SockImpl.c > | 4 +- > NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c > | 2 +- > OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c > | 6 +- > OvmfPkg/PlatformDxe/Platform.c > | 4 +- > OvmfPkg/VirtioNetDxe/Events.c > | 2 +- > OvmfPkg/XenBusDxe/XenBus.c > | 2 +- > SecurityPkg/HddPassword/HddPasswordDxe.c > | 2 +- > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigDriver.c > | 2 +- > SecurityPkg/Tcg/TcgConfigDxe/TcgConfigDriver.c > | 2 +- > > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDriver.c > | 2 +- > ShellPkg/DynamicCommand/DpDynamicCommand/Dp.c > | 6 +- > ShellPkg/DynamicCommand/DpDynamicCommand/Dp.h > | 4 +- > ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c > | 6 +- > ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.h > | 4 +- > ShellPkg/Include/Library/ShellCommandLib.h > | 2 +- > ShellPkg/Include/Library/ShellLib.h > | 4 +- > ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c > | 2 +- > ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c > | 2 +- > ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c > | 2 +- > ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.h > | 2 +- > ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/FileBuffer.c > | 2 +- > ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.c > | 2 +- > ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.h > | 2 +- > ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c > | 4 +- > ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.c > | 2 +- > ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.h > | 2 +- > ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.c > | 2 +- > ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1CommandsLib.h > | 2 +- > ShellPkg/Library/UefiShellLevel2CommandsLib/Rm.c > | 2 +- > ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.c > | 2 +- > ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2CommandsLib.h > | 2 +- > ShellPkg/Library/UefiShellLevel3CommandsLib/Touch.c > | 2 +- > ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.c > | 2 +- > ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.h > | 2 +- > ShellPkg/Library/UefiShellLib/UefiShellLib.c > | 26 ++++++- > > ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.c > | 2 +- > > ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.h > | 2 +- > > ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.c > | 2 +- > > ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.h > | 2 +- > StandaloneMmPkg/Core/Dispatcher.c > | 80 +++++++++++--------- > StandaloneMmPkg/Core/FwVol.c > | 16 ++-- > StandaloneMmPkg/Core/StandaloneMmCore.h > | 4 +- > UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c > | 4 +- > UefiPayloadPkg/BlSupportPei/BlSupportPei.c > | 19 +++-- > 102 files changed, 294 insertions(+), 194 deletions(-) > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47861): https://edk2.groups.io/g/devel/message/47861 Mute This Topic: https://groups.io/mt/34180197/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
