Re: [edk2-devel] [PATCH 00/35] edk2: clean up the usage of standardized (VOID*) typedefs

["Marvin Häuser" <mhaeuser@outlook.de>]

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.gupta@arm.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Benjamin You <benjamin.you@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Guo Dong <guo.dong@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jian Wang <jian.j.wang@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Maurice Ma <maurice.ma@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
> Cc: Zhichao Gao <zhichao.gao@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(-)
>