-
Notifications
You must be signed in to change notification settings - Fork 2.8k
MdeModulePkg: DebugImageInfoTable: Fix Array Maintenance #11013
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The DebugImageInfoTable contains an array of image info structures. The current implementation removes an entry by freeing the info structure and putting NULL in that entry of the array. It then decrements the table size tracked in the table. However, the array is invalid at this point, it contains a NULL entry, which the UEFI spec does not envision and it contains a valid entry past the end of the array as tracked in the spec defined config table. If the table is consumed at this point it can lead to an invalid assessment of the image state, which defeats the purpose of the table. When a new info structure is added, it then scans for the first NULL entry adds a pointer to the new info structure there and increments the table size to cover the entrythat was formerly past the end of the array. The current implementation requires that once an unload happens, more loads happen than unloads and that the last operation is not an unload (which won't be true in the shell, e.g.). This is needlessly complex, as the order of the table doesn't matter (and in fact this implementation doesn't preserve image loading order either). This patch updates the removal function to free the desired info structure, move the last entry of the array to this freed spot, mark the last entry as NULL, and decrement the table count. The entry addition function then just always puts a new entry at the end of the array, expanding it as necessary. This simplifies the logic and covers the gaps that were present. Signed-off-by: Oliver Smith-Denny <[email protected]>
Tested-by @xypron |
Having the entries in load order would be convenient for debugging, but this feature was neither provided by the previous implementation nor is it required by the UEFI specification, cf #11019. |
Yeah, my reasoning was that image load order is fairly easy to get from the log and as you said, neither the spec nor the existing implementation provided for it, so I thought speed was more important here (especially as on most boots this table will not be used, but still produced). |
@lgao4 can you please review this PR as it was put up before the soft freeze? |
Provide a test application to dump the EFI_DEBUG_IMAGE_INFO_TABLE as implemented in EDK II. EFI_DEBUG_IMAGE_INFO is not packed in contrast to many other EFI structures. As of today EDK II when removing an entry in the EfiDebugImageInfoTable just sets NormalImage = NULL but does not compact the array. So TableSize reflects the number of non-NULL entries and not the array size as reported independently in tianocore/edk2#11013 and tianocore/edk2#11019. The current implementation tolerates this deviation from the UEFI specification. This is what the output may look like: Debug Info Table Dump ===================== => dump Modified Number of entries: 0x0000004a Info type 0x00000001 Address: [0x000000008315a000, 0x00000000831bafff] File: FvFile(D6A2CB7F-6A18-4E2F-B43B-9920A733700A) Handle: 0x000000017fe3cb18 ... Info type 0x00000001 Address: [0x000000017e8db000, 0x000000017ea00f3f] File: FvFile(7C04A583-9E3E-4F1C-AD65-E05268D0B4D1) Handle: 0x000000017f358e98 Info type 0x00000001 Address: [0x000000017eae5000, 0x000000017eae81ff] File: \dbginfodump.efi Handle: 0x000000017eaf0298 => Signed-off-by: Heinrich Schuchardt <[email protected]> Acked-by: Ilias Apalodimas <[email protected]> Signed-off-by: Ilias Apalodimas <[email protected]>
Description
The DebugImageInfoTable contains an array of image info structures. The current implementation removes an entry by freeing the info structure and putting NULL in that entry of the array. It then decrements the table size tracked in the table. However, the array is invalid at this point, it contains a NULL entry, which the UEFI spec does not envision and it contains a valid entry past the end of the array as tracked in the spec defined config table. If the table is consumed at this point it can lead to an invalid assessment of the image state, which defeats the purpose of the table.
When a new info structure is added, it then scans for the first NULL entry adds a pointer to the new info structure there and increments the table size to cover the entrythat was formerly past the end of the array.
The current implementation requires that once an unload happens, more loads happen than unloads and that the last operation is not an unload (which won't be true in the shell, e.g.). This is needlessly complex, as the order of the table doesn't matter (and in fact this implementation doesn't preserve image loading order either).
This patch updates the removal function to free the desired info structure, move the last entry of the array to this freed spot, mark the last entry as NULL, and decrement the table count. The entry addition function then just always puts a new entry at the end of the array, expanding it as necessary. This simplifies the logic and covers the gaps that were present.
How This Was Tested
Tested by running a shell app that uses the DebugImageInfoTable and confirmed it could access all expected entries.
Integration Instructions
N/A.