MdePkg: pack EFI_FILE_INFO

[kraxel]

• MdePkg: pack EFI_FILE_INFO
• Revert "OvmfPkg/QemuKernelLoaderFsDxe: drop bogus assert"

[kraxel]

@lersek

[mdkinney]

This is not the way byte packing is applied to structures in rest of edk2. #pragma pack() is used and should only be used if the UEFI Spec states that it should be byte packed.

[lersek]

@mdkinney Hi Mike, I agree about #pragma pack (1) of course. Regarding the spec, members of the structure are already naturally aligned (and the spec requires natural alignment by default), so packing should have no effect other than removing the trailing padding (which is the intent). The wording of the spec, and the comment at the end of MdePkg/Include/Guid/FileInfo.h, near SIZE_OF_EFI_FILE_INFO, suggest that trailing padding was never intended for this structure.

There are two problems with EFI_FILE_INFO having trailing padding (with either issue occurring when the FileName member terminates before the trailing padding of EFI_FILE_INFO does):

• The Size member may be set to a value smaller than sizeof (EFI_FILE_INFO), which is minimally weird (Size should never indicate truncation).
• If EFI_FILE_PROTOCOL.GetInfo(... &gEfiFileInfoGuid ...) is invoked with a zero BufferSize at first, and the output BufferSize is less than sizeof (EFI_FILE_INFO), then the subequent allocation will not provide enough space for a whole EFI_FILE_INFO object. Subsequently, accesses like FileInfo->Attribute, which is equivalent to (*FileInfo).Attribute, may violate the compiler's expectation that FileInfo point at an object with at least sizeof *FileInfo bytes.

[mdkinney]

use of sizeof() on a structure that uses an array of [1] at the end to indicate a flexible array member is not correct. Instead, the OFFSET_OF() macro should be used to determine the size of a structure declared in this way.

Of course proper use of flexible array member would be much cleaner. However, some of the UEFI Spec structures were defined before flexible array members would available in all compilers and C standards.

[lersek]

@mdkinney Hi Mike, sizeof *FileInfo is intentional here. The ASSERT() expresses that the invariable parts of EFI_FILE_INFO, plus the variable/flexible ("struct hack") FileName mmber, are not shorter than the size of EFI_FILE_INFO. In other words, that EFI_FILE_INFO does not get truncated due to a short FileName.

Of course proper use of flexible array member would be much cleaner. However, some of the UEFI Spec structures were defined before flexible array members would available in all compilers and C standards.

This is (independently) highly relevant, as the UEFI spec actually declares EFI_FILE_INFO.FileName as a (C99) flexible array member, but edk2 declares the same member as a 1-element array (C89 struct hack).

[ardbiesheuvel]

Struct packing also has impact on alignment, and on some architectures, the codegen will be much worse because the compiler will have to assume that a EFI_FILE_INFO may appear misaligned in memory.

Could we simply bring EFI_FILE_INFO in line with the spec, and turn it into a true flex array?

[mdkinney]

I see the updated commit to change FileName[] to a flexible array member, which makes sizeof (EFI_FILE_INFO) a smaller value. The entire repo needs to be reviewed for use of sizeof (EFI_FILE_INFO). There is a macro called SIZE_OF_EFI_FILE_INFO that is used in some places and that is correct. With the change to a flexible array member, SIZE_OF_EFI_FILE_INFO can be changed from using OFFSET_OF to using sizeof().

For example, I think the subtraction of sizeof (CHAR16) in the following line is now incorrect:

edk2/MdeModulePkg/Universal/FvSimpleFileSystemDxe/FvSimpleFileSystemEntryPoint.c

Line 205 in feb8d49

FvFileInfo->FileInfo.Size = sizeof (EFI_FILE_INFO) + NameLen - sizeof (CHAR16);