[lib][uefi] Fix memory leak in load_pe_file by zhangxp1998 · Pull Request #468 · littlekernel/lk

zhangxp1998 · 2025-08-26T19:41:04Z

No description provided.

lib/uefi/uefi.cpp

schultetwin1

One small change but looks good

schultetwin1 · 2025-09-03T13:32:12Z

lib/uefi/uefi.cpp

  if (!dev) {
    printf("error opening block device %s\n", blkdev);
-    return -1;
+    return ERR_IO;


The only reason bio_open will return NULL is due to ERR_NOT_FOUND, so let's change this.

Suggested change

return ERR_IO;

return ERR_NOT_FOUND;

Copilot

Pull request overview

This PR aims to fix a memory leak in the PE loader (load_pe_file) and refactors/extends the UEFI/GBL interface layer (protocol headers, types, and relocation handling).

Changes:

Fix load_pe_file heap leak by freeing the PE header buffer and add basic I/O error handling.
Update multiple UEFI/GBL protocol headers (signatures, revisions, and type cleanups) and add several new protocol/type headers.
Extend relocation handling with a new relocation type and adjust charset conversion APIs.

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
lib/uefi/uefi_platform.h	Reorders platform hook function parameters to match updated protocol definitions.
lib/uefi/uefi_platform.cpp	Updates weak platform hook implementations to match new signatures.
lib/uefi/uefi.cpp	Fixes PE header buffer leak; adds read checks; updates vendor type usage and error codes.
lib/uefi/runtime_service_provider.cpp	Switches variable-name handling to UTF-16 helpers and adjusts logging.
lib/uefi/relocation.cpp	Removes ARM MOV32T support and adds a LoongArch64 relocation case.
lib/uefi/pe.h	Adds relocation constant and a comment for PE signature.
lib/uefi/include/uefi/system_table.h	Adjusts system table vendor field type and include style.
lib/uefi/include/uefi/runtime_service.h	Adjusts include style to `<uefi/types.h>`.
lib/uefi/include/uefi/protocols/gbl_efi_os_configuration_protocol.h	Changes protocol revision and reorders/annotates function parameters.
lib/uefi/include/uefi/protocols/gbl_efi_image_loading_protocol.h	Changes protocol revision constant form and adjusts string field types.
lib/uefi/include/uefi/protocols/gbl_efi_fastboot_usb.h	Adds new GBL fastboot USB protocol definition.
lib/uefi/include/uefi/protocols/gbl_efi_fastboot_transport_protocol.h	Adds new transport protocol and RX mode enum.
lib/uefi/include/uefi/protocols/gbl_efi_fastboot_protocol.h	Updates fastboot protocol revision and API surface.
lib/uefi/include/uefi/protocols/gbl_efi_debug_protocol.h	Bumps protocol revision and adds parameter direction annotations.
lib/uefi/include/uefi/protocols/gbl_efi_boot_memory_protocol.h	Bumps protocol revision.
lib/uefi/include/uefi/protocols/gbl_efi_boot_control_protocol.h	Adjusts slot info fields and changes `handle_loaded_os` signature.
lib/uefi/include/uefi/protocols/gbl_efi_avf_protocol.h	Bumps AVF protocol revision.
lib/uefi/include/uefi/protocols/gbl_efi_avb_protocol.h	Expands AVB status/flags/types and renames API/structs.
lib/uefi/include/uefi/protocols/gbl_efi_ab_slot_protocol.h	Adds new AB slot protocol definition.
lib/uefi/include/types.h	Adds a “flat” EFI types header (parallel to `<uefi/types.h>`).
lib/uefi/include/system_table.h	Adds a “flat” system table header (parallel to `<uefi/system_table.h>`).
lib/uefi/include/runtime_service.h	Adds a “flat” runtime service header (parallel to `<uefi/runtime_service.h>`).
lib/uefi/include/gbl_protocol_utils.h	Adds protocol revision macros and an `EFI_ENUM` macro.
lib/uefi/include/gbl_efi_common.h	Adds shared GUID/constants for GBL EFI integration.
lib/uefi/include/efi.h	Adds umbrella header intended to pull in the full EFI/protocol surface.
lib/uefi/include/boot_service.h	Adds a “flat” boot services table header (parallel to `<uefi/boot_service.h>`).
lib/uefi/charset.h	Adds an overload helper and changes conversion function return types.
lib/uefi/charset.cpp	Implements conversion functions with `size_t` return type and returns converted length.
lib/uefi/boot_service_provider.cpp	Adds allocation failure handling and adjusts one protocol handling return.
.clang-format	Formatting tweak to macro alignment indentation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-25T22:52:12Z

lib/uefi/include/efi.h

+#include "protocols/block_io2_protocol.h"
+#include "protocols/block_io_protocol.h"
+#include "protocols/device_path_protocol.h"
+#include "protocols/dt_fixup_protocol.h"
+#include "protocols/erase_block_protocol.h"
+#include "protocols/gbl_efi_avb_protocol.h"
+#include "protocols/gbl_efi_avf_protocol.h"
+#include "protocols/gbl_efi_boot_control_protocol.h"
+#include "protocols/gbl_efi_boot_memory_protocol.h"
+#include "protocols/gbl_efi_debug_protocol.h"
+#include "protocols/gbl_efi_fastboot_protocol.h"
+#include "protocols/gbl_efi_fastboot_transport_protocol.h"
+#include "protocols/gbl_efi_os_configuration_protocol.h"
+#include "protocols/hash2_protocol.h"
+#include "protocols/loaded_image_protocol.h"
+#include "protocols/random_number_generator_protocol.h"
+#include "protocols/riscv_efi_boot_protocol.h"
+#include "protocols/service_binding_protocol.h"
+#include "protocols/simple_network_protocol.h"
+#include "protocols/simple_text_input_protocol.h"
+#include "protocols/simple_text_output_protocol.h"
+#include "protocols/timestamp.h"


This header includes "protocols/..." headers (e.g. block I/O) but there is no lib/uefi/include/protocols/ directory in the repo (protocol headers live under lib/uefi/include/uefi/protocols/). As written, including efi.h will fail unless the build adds a non-obvious include path. Update these includes to match the actual layout (e.g., <uefi/protocols/...>).

Suggested change

#include "protocols/block_io2_protocol.h"

#include "protocols/block_io_protocol.h"

#include "protocols/device_path_protocol.h"

#include "protocols/dt_fixup_protocol.h"

#include "protocols/erase_block_protocol.h"

#include "protocols/gbl_efi_avb_protocol.h"

#include "protocols/gbl_efi_avf_protocol.h"

#include "protocols/gbl_efi_boot_control_protocol.h"

#include "protocols/gbl_efi_boot_memory_protocol.h"

#include "protocols/gbl_efi_debug_protocol.h"

#include "protocols/gbl_efi_fastboot_protocol.h"

#include "protocols/gbl_efi_fastboot_transport_protocol.h"

#include "protocols/gbl_efi_os_configuration_protocol.h"

#include "protocols/hash2_protocol.h"

#include "protocols/loaded_image_protocol.h"

#include "protocols/random_number_generator_protocol.h"

#include "protocols/riscv_efi_boot_protocol.h"

#include "protocols/service_binding_protocol.h"

#include "protocols/simple_network_protocol.h"

#include "protocols/simple_text_input_protocol.h"

#include "protocols/simple_text_output_protocol.h"

#include "protocols/timestamp.h"

#include <uefi/protocols/block_io2_protocol.h>

#include <uefi/protocols/block_io_protocol.h>

#include <uefi/protocols/device_path_protocol.h>

#include <uefi/protocols/dt_fixup_protocol.h>

#include <uefi/protocols/erase_block_protocol.h>

#include <uefi/protocols/gbl_efi_avb_protocol.h>

#include <uefi/protocols/gbl_efi_avf_protocol.h>

#include <uefi/protocols/gbl_efi_boot_control_protocol.h>

#include <uefi/protocols/gbl_efi_boot_memory_protocol.h>

#include <uefi/protocols/gbl_efi_debug_protocol.h>

#include <uefi/protocols/gbl_efi_fastboot_protocol.h>

#include <uefi/protocols/gbl_efi_fastboot_transport_protocol.h>

#include <uefi/protocols/gbl_efi_os_configuration_protocol.h>

#include <uefi/protocols/hash2_protocol.h>

#include <uefi/protocols/loaded_image_protocol.h>

#include <uefi/protocols/random_number_generator_protocol.h>

#include <uefi/protocols/riscv_efi_boot_protocol.h>

#include <uefi/protocols/service_binding_protocol.h>

#include <uefi/protocols/simple_network_protocol.h>

#include <uefi/protocols/simple_text_input_protocol.h>

#include <uefi/protocols/simple_text_output_protocol.h>

#include <uefi/protocols/timestamp.h>

Copilot · 2026-02-25T22:52:13Z

lib/uefi/uefi.cpp

+  }
+  if (file_header->Machine != ArchitectureType::ARM64) {
+    printf("Unsupported PE header machine type: %x\n",
+           static_cast<int>(file_header->Machine));


When file_header->Machine is not ARM64, the code prints "Unsupported" but continues parsing/loading anyway. This can lead to executing an image for the wrong architecture. Return ERR_NOT_SUPPORTED (or equivalent) after printing the message to fail fast.

Suggested change

static_cast<int>(file_header->Machine));

static_cast<int>(file_header->Machine));

return ERR_NOT_SUPPORTED;

Copilot · 2026-02-25T22:52:13Z

lib/uefi/include/uefi/protocols/gbl_efi_ab_slot_protocol.h

+#ifndef __GBL_EFI_AB_SLOT_PROTOCOL_H__
+#define __GBL_EFI_AB_SLOT_PROTOCOL_H__
+
+#include "system_table.h"


This header includes "system_table.h", which is not used anywhere in the file and is ambiguous given there are multiple system_table.h headers in the tree. It also risks pulling in the wrong definitions depending on include paths. Remove this include (or change it to the intended <uefi/system_table.h> if actually needed).

Suggested change

#include "system_table.h"

Copilot · 2026-02-25T22:52:13Z

lib/uefi/relocation.cpp

+
+        Fixup += sizeof(uint32_t);
+        *Fixup32 = (*Fixup32 & ~0x3ffc00) | (((Value >> 52) & 0xfff) << 10);
+      }


This case EFI_IMAGE_REL_BASED_LOONGARCH64_MARK_LA block falls through into default because it lacks a break; after the closing brace. As written it will always print "Unsupported relocation type" and return -1 even after performing the relocation.

Suggested change

}

}

break;

Copilot · 2026-02-25T22:52:14Z

lib/uefi/runtime_service_provider.cpp

-  }
-  buffer[i] = 0;
-  if (strncmp(buffer, kSecureBoot, sizeof(kSecureBoot)) == 0 || strcmp(buffer, "SetupMode") == 0) {
+  if (utf16_strcmp(VariableName, u"SecureBoot") == 0 &&


The SecureBoot/SetupMode special-case condition is impossible to satisfy because it uses && while comparing VariableName against two different strings. This means the branch never runs and these variables fall through to the unsupported path. Use || so either name matches.

Suggested change

if (utf16_strcmp(VariableName, u"SecureBoot") == 0 &&

if (utf16_strcmp(VariableName, u"SecureBoot") == 0 ||

Copilot · 2026-02-25T22:52:14Z

lib/uefi/charset.h

+static inline int utf16_strcmp(const uint16_t *s1, const char16_t *s2) {
+  return utf16_strcmp(reinterpret_cast<const char16_t *>(s1), s2);
+}
+// Return number of characters converted, including the null terminator


The comment says the conversion functions return the number of characters converted "including the null terminator", but the implementation returns the count of non-null characters copied (the terminator is written after the loop and not counted). Update the comment or adjust the return value for consistency.

Suggested change

// Return number of characters converted, including the null terminator

// Return number of characters converted, excluding the null terminator

Copilot · 2026-02-25T22:52:14Z

lib/uefi/uefi.cpp

-  reader->read(image_base, 0, section_header[0].PointerToRawData);
+  ssize_t bytes_read =
+      reader->read(image_base, 0, section_header[0].PointerToRawData);
+  if (bytes_read != section_header[0].SizeOfRawData) {


The initial read requests section_header[0].PointerToRawData bytes but compares bytes_read to section_header[0].SizeOfRawData. These values typically differ, so this check will incorrectly fail and abort PE loading. Compare against the requested length (PointerToRawData).

Suggested change

if (bytes_read != section_header[0].SizeOfRawData) {

if (bytes_read != static_cast<ssize_t>(section_header[0].PointerToRawData)) {

Copilot · 2026-02-25T22:52:14Z

lib/uefi/include/gbl_efi_common.h

+    .data1=0x5a6d92f3,                                        \
+    .data2=0xa2d0,                                            \
+    .data3=0x4083,                                            \
+    .data4=[0x91, 0xa1, 0xa5, 0x0f, 0x6c, 0x3d, 0x98, 0x30]   \


The GUID macro initializes data4 using square brackets (.data4=[...]), which is not valid array initialization syntax and will fail to compile. Use a brace initializer for the array member (e.g., .data4 = { ... }).

Suggested change

.data4=[0x91, 0xa1, 0xa5, 0x0f, 0x6c, 0x3d, 0x98, 0x30] \

.data4 = {0x91, 0xa1, 0xa5, 0x0f, 0x6c, 0x3d, 0x98, 0x30} \

Copilot · 2026-02-25T22:52:15Z

lib/uefi/include/system_table.h

+
+typedef struct EfiSystemTable {
+  EfiTableHeader header;
+  uint16_t* firmware_vendor;


EfiSystemTable is defined differently here than in lib/uefi/include/uefi/system_table.h (e.g., firmware_vendor type/constness). Two incompatible definitions with the same type name are error-prone and can break builds if both headers are included together. Consider consolidating on one definition or preventing these headers from being mixed.

Suggested change

uint16_t* firmware_vendor;

const uint16_t* firmware_vendor;

Copilot · 2026-02-25T22:52:15Z

lib/uefi/relocation.cpp

+        Fixup += sizeof(uint32_t);
+        *Fixup32 = (*Fixup32 & ~0x3ffc00) | ((Value & 0xfff) << 10);
+
+        Fixup += sizeof(uint32_t);
+        *Fixup32 = (*Fixup32 & ~0x1ffffe0) | (((Value >> 32) & 0xfffff) << 5);
+
+        Fixup += sizeof(uint32_t);


In this LOONGARCH64 relocation case, Fixup is advanced but stores still go through the original Fixup32 pointer, so the later writes will patch the wrong instruction word(s). Update the pointer used for stores (e.g., increment Fixup32 / recompute it from Fixup) so each of the 4 instructions is patched correctly.

Suggested change

Fixup += sizeof(uint32_t);

*Fixup32 = (*Fixup32 & ~0x3ffc00) | ((Value & 0xfff) << 10);

Fixup += sizeof(uint32_t);

*Fixup32 = (*Fixup32 & ~0x1ffffe0) | (((Value >> 32) & 0xfffff) << 5);

Fixup += sizeof(uint32_t);

Fixup += sizeof(uint32_t);

Fixup32 = reinterpret_cast<uint32_t *>(Fixup);

*Fixup32 = (*Fixup32 & ~0x3ffc00) | ((Value & 0xfff) << 10);

Fixup += sizeof(uint32_t);

Fixup32 = reinterpret_cast<uint32_t *>(Fixup);

*Fixup32 = (*Fixup32 & ~0x1ffffe0) | (((Value >> 32) & 0xfffff) << 5);

Fixup += sizeof(uint32_t);

Fixup32 = reinterpret_cast<uint32_t *>(Fixup);

travisg · 2026-02-25T22:55:00Z

Oh huh, I didn't know about this Copilot review thing, it must have been kicked in. Well, take what you need out of it, there's probably some actual useful stuff in there.

relocation

schultetwin1 reviewed Aug 26, 2025

View reviewed changes

lib/uefi/uefi.cpp Outdated Show resolved Hide resolved

zhangxp1998 force-pushed the master branch from 8238033 to 663512c Compare August 26, 2025 20:13

schultetwin1 approved these changes Sep 3, 2025

View reviewed changes

zhangxp1998 force-pushed the master branch 3 times, most recently from 03f8336 to 3e73cb0 Compare September 3, 2025 18:03

zhangxp1998 force-pushed the master branch from c59da84 to f152027 Compare December 8, 2025 19:21

Copilot AI review requested due to automatic review settings February 25, 2026 22:43

zhangxp1998 force-pushed the master branch from f152027 to a629244 Compare February 25, 2026 22:43

Copilot started reviewing on behalf of zhangxp1998 February 25, 2026 22:44 View session

zhangxp1998 force-pushed the master branch from a629244 to 0e48670 Compare February 25, 2026 22:45

Copilot AI reviewed Feb 25, 2026

View reviewed changes

zhangxp1998 force-pushed the master branch from 0e48670 to ef4a497 Compare February 25, 2026 22:58

zhangxp1998 closed this Mar 2, 2026

zhangxp1998 reopened this Mar 2, 2026

zhangxp1998 added 5 commits March 2, 2026 12:33

[lib][uefi] Fix memory leak in load_pe_file

bc39f4e

[lib][uefi] Use LK standard error codes for uefi.cpp

6416056

[lib][uefi] Add error checking for certain functions

1aed710

[lib][uefi] Use utf16 charset functions

f947ab7

[lib][uefi] Remove THUMB mode relocation, added aarch64 specific

f103ce0

relocation

zhangxp1998 force-pushed the master branch from ef4a497 to f103ce0 Compare March 2, 2026 20:40

zhangxp1998 closed this Mar 2, 2026

zhangxp1998 mentioned this pull request Mar 2, 2026

[lib][uefi] Fix memory leak in load_pe_file #497

Merged

	static_cast<int>(file_header->Machine));
	static_cast<int>(file_header->Machine));
	return ERR_NOT_SUPPORTED;

	if (utf16_strcmp(VariableName, u"SecureBoot") == 0 &&
	if (utf16_strcmp(VariableName, u"SecureBoot") == 0 \|\|

	// Return number of characters converted, including the null terminator
	// Return number of characters converted, excluding the null terminator

	if (bytes_read != section_header[0].SizeOfRawData) {
	if (bytes_read != static_cast<ssize_t>(section_header[0].PointerToRawData)) {

	.data4=[0x91, 0xa1, 0xa5, 0x0f, 0x6c, 0x3d, 0x98, 0x30] \
	.data4 = {0x91, 0xa1, 0xa5, 0x0f, 0x6c, 0x3d, 0x98, 0x30} \

Conversation

zhangxp1998 commented Aug 26, 2025

Uh oh!

Uh oh!

schultetwin1 left a comment

Choose a reason for hiding this comment

Uh oh!

schultetwin1 Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

travisg commented Feb 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants