From a9b0654a59ebde87a4f2beaa0c0e4e57be57826c Mon Sep 17 00:00:00 2001 From: jief Date: Sun, 12 Nov 2023 03:48:29 +0100 Subject: [PATCH] Improve XString forgetDataWithoutFreeing() to avoid freeing a literal. Improve XString stealValueFrom to avoid a memory leak Change GlobalConfig.ACPIDropTables to a XObjArray. --- .../UefiMock/Library/MemoryAllocationLib.c | 2 +- Xcode/cpp_tests/src/main.cpp | 10 +++++ rEFIt_UEFI/Platform/AcpiPatcher.cpp | 39 +++++++++++-------- rEFIt_UEFI/Platform/Settings.cpp | 20 +++++----- rEFIt_UEFI/Platform/Settings.h | 15 ++++--- rEFIt_UEFI/cpp_foundation/XStringAbstract.h | 9 ++++- rEFIt_UEFI/cpp_foundation/XStringArray.h | 12 +++++- rEFIt_UEFI/libeg/XTheme.h | 8 +++- rEFIt_UEFI/libeg/nanosvg.cpp | 2 +- rEFIt_UEFI/refit/menu.cpp | 27 +++++++------ 10 files changed, 88 insertions(+), 56 deletions(-) diff --git a/PosixCompilation/UefiMock/Library/MemoryAllocationLib.c b/PosixCompilation/UefiMock/Library/MemoryAllocationLib.c index f29f6d6d9..c769d0f4e 100644 --- a/PosixCompilation/UefiMock/Library/MemoryAllocationLib.c +++ b/PosixCompilation/UefiMock/Library/MemoryAllocationLib.c @@ -35,7 +35,7 @@ void* ReallocatePool(UINTN OldSize, UINTN NewSize, void* OldBuffer) return (void*)realloc(OldBuffer, (size_t)NewSize); } -void FreePool(IN JCONST VOID *Buffer) +void FreePool(IN VOID *Buffer) { free((void*)Buffer); } diff --git a/Xcode/cpp_tests/src/main.cpp b/Xcode/cpp_tests/src/main.cpp index 1bddebcd1..fe98c179c 100755 --- a/Xcode/cpp_tests/src/main.cpp +++ b/Xcode/cpp_tests/src/main.cpp @@ -127,6 +127,16 @@ extern "C" int main(int argc, const char * argv[]) (void)argv; setlocale(LC_ALL, "en_US"); // to allow printf unicode char + XString8 s("foo"); +// s.strcat("a"); + char* p = s.forgetDataWithoutFreeing(); + free(p); + + XString8 s2; +// s2.S8Printf("bar"); + char* q = s2.forgetDataWithoutFreeing(); + free(q); + MyFloat test = 5.0f; MutableRef Background; diff --git a/rEFIt_UEFI/Platform/AcpiPatcher.cpp b/rEFIt_UEFI/Platform/AcpiPatcher.cpp index b425c4677..b22860d55 100644 --- a/rEFIt_UEFI/Platform/AcpiPatcher.cpp +++ b/rEFIt_UEFI/Platform/AcpiPatcher.cpp @@ -254,8 +254,7 @@ void AddDropTable(EFI_ACPI_DESCRIPTION_HEADER* Table, UINT32 Index) DropTable->TableId = Table->OemTableId; DropTable->Length = Table->Length; DropTable->MenuItem.BValue = false; - DropTable->Next = GlobalConfig.ACPIDropTables; - GlobalConfig.ACPIDropTables = DropTable; + GlobalConfig.ACPIDropTables.AddReference(DropTable, true); } @@ -284,7 +283,7 @@ void GetAcpiTablesList() DbgHeader("GetAcpiTablesList"); GetFadt(); //this is a first call to acpi, we need it to make a pointer to Xsdt - GlobalConfig.ACPIDropTables = NULL; + GlobalConfig.ACPIDropTables.setEmpty(); DBG("Get Acpi Tables List "); /* @@ -2137,14 +2136,14 @@ EFI_STATUS PatchACPI(IN REFIT_VOLUME *Volume, const MacOsVersion& OSVersion) LoadAllPatchedAML(L"ACPI\\patched"_XSW, AUTOMERGE_PASS1); // Drop tables - if (GlobalConfig.ACPIDropTables) { - ACPI_DROP_TABLE *DropTable; + if (GlobalConfig.ACPIDropTables.notEmpty()) { DbgHeader("ACPIDropTables"); - for (DropTable = GlobalConfig.ACPIDropTables; DropTable; DropTable = DropTable->Next) { - if (DropTable->MenuItem.BValue) { - //DBG("Attempting to drop \"%4.4a\" (%8.8X) \"%8.8a\" (%16.16lX) L=%d\n", &(DropTable->Signature), DropTable->Signature, &(DropTable->TableId), DropTable->TableId, DropTable->Length); - DropTableFromXSDT(DropTable->Signature, DropTable->TableId, DropTable->Length); - DropTableFromRSDT(DropTable->Signature, DropTable->TableId, DropTable->Length); + for ( size_t idx = 0 ; idx < GlobalConfig.ACPIDropTables.length() ; ++idx ) { + ACPI_DROP_TABLE& DropTable = GlobalConfig.ACPIDropTables[idx]; + if (DropTable.MenuItem.BValue) { + //DBG("Attempting to drop \"%4.4a\" (%8.8X) \"%8.8a\" (%16.16lX) L=%d\n", &(DropTable.Signature), DropTable.Signature, &(DropTable.TableId), DropTable.TableId, DropTable.Length); + DropTableFromXSDT(DropTable.Signature, DropTable.TableId, DropTable.Length); + DropTableFromRSDT(DropTable.Signature, DropTable.TableId, DropTable.Length); } } } @@ -2563,15 +2562,21 @@ EFI_STATUS PatchACPI_OtherOS(CONST CHAR16* OsSubdir, XBool DropSSDT) DropTableFromRSDT(EFI_ACPI_4_0_SECONDARY_SYSTEM_DESCRIPTION_TABLE_SIGNATURE, 0, 0); } */ - if (GlobalConfig.ACPIDropTables) { - ACPI_DROP_TABLE *DropTable; + if (GlobalConfig.ACPIDropTables.notEmpty()) { + for ( size_t idx = 0 ; idx < GlobalConfig.ACPIDropTables.length() ; ++idx ) { + ACPI_DROP_TABLE& DropTable = GlobalConfig.ACPIDropTables[idx]; + DropTable.MenuItem.ItemType = BoolValue; + } + } + if (GlobalConfig.ACPIDropTables.notEmpty()) { DbgHeader("ACPIDropTables"); - for (DropTable = GlobalConfig.ACPIDropTables; DropTable; DropTable = DropTable->Next) { + for ( size_t idx = 0 ; idx < GlobalConfig.ACPIDropTables.length() ; ++idx ) { + ACPI_DROP_TABLE& DropTable = GlobalConfig.ACPIDropTables[idx]; // only for tables that have OtherOS true - if (DropTable->OtherOS && DropTable->MenuItem.BValue) { - //DBG("Attempting to drop \"%4.4a\" (%8.8X) \"%8.8a\" (%16.16lX) L=%d\n", &(DropTable->Signature), DropTable->Signature, &(DropTable->TableId), DropTable->TableId, DropTable->Length); - DropTableFromXSDT(DropTable->Signature, DropTable->TableId, DropTable->Length); - DropTableFromRSDT(DropTable->Signature, DropTable->TableId, DropTable->Length); + if (DropTable.OtherOS && DropTable.MenuItem.BValue) { + //DBG("Attempting to drop \"%4.4a\" (%8.8X) \"%8.8a\" (%16.16lX) L=%d\n", &(DropTable.Signature), DropTable.Signature, &(DropTable.TableId), DropTable.TableId, DropTable.Length); + DropTableFromXSDT(DropTable.Signature, DropTable.TableId, DropTable.Length); + DropTableFromRSDT(DropTable.Signature, DropTable.TableId, DropTable.Length); } } } diff --git a/rEFIt_UEFI/Platform/Settings.cpp b/rEFIt_UEFI/Platform/Settings.cpp index 34e0e4f03..eb9ec9a6d 100755 --- a/rEFIt_UEFI/Platform/Settings.cpp +++ b/rEFIt_UEFI/Platform/Settings.cpp @@ -334,27 +334,25 @@ void afterGetUserSettings(SETTINGS_DATA& settingsData) GlobalConfig.SecureBoot = 1; } - //set to drop GlobalConfig.DropSSDT = settingsData.ACPI.SSDT.DropSSDTSetting; - if (GlobalConfig.ACPIDropTables) { + if (GlobalConfig.ACPIDropTables.notEmpty()) { for ( size_t idx = 0 ; idx < settingsData.ACPI.ACPIDropTablesArray.size() ; ++idx) { - ACPI_DROP_TABLE *DropTable = GlobalConfig.ACPIDropTables; DBG(" - [%02zd]: Drop table : %.4s, %16llx : ", idx, (const char*)&settingsData.ACPI.ACPIDropTablesArray[idx].Signature, settingsData.ACPI.ACPIDropTablesArray[idx].TableId); XBool Dropped = false; - while (DropTable) { - if (((settingsData.ACPI.ACPIDropTablesArray[idx].Signature == DropTable->Signature) && - (!settingsData.ACPI.ACPIDropTablesArray[idx].TableId || (DropTable->TableId == settingsData.ACPI.ACPIDropTablesArray[idx].TableId)) && - (!settingsData.ACPI.ACPIDropTablesArray[idx].TabLength || (DropTable->Length == settingsData.ACPI.ACPIDropTablesArray[idx].TabLength))) || - (!settingsData.ACPI.ACPIDropTablesArray[idx].Signature && (DropTable->TableId == settingsData.ACPI.ACPIDropTablesArray[idx].TableId))) { - DropTable->MenuItem.BValue = true; - DropTable->OtherOS = settingsData.ACPI.ACPIDropTablesArray[idx].OtherOS; + for ( size_t idx2 = 0 ; idx2 < GlobalConfig.ACPIDropTables.length() ; ++idx2 ) { + ACPI_DROP_TABLE& DropTable = GlobalConfig.ACPIDropTables[idx2]; + if (((settingsData.ACPI.ACPIDropTablesArray[idx].Signature == DropTable.Signature) && + (!settingsData.ACPI.ACPIDropTablesArray[idx].TableId || (DropTable.TableId == settingsData.ACPI.ACPIDropTablesArray[idx].TableId)) && + (!settingsData.ACPI.ACPIDropTablesArray[idx].TabLength || (DropTable.Length == settingsData.ACPI.ACPIDropTablesArray[idx].TabLength))) || + (!settingsData.ACPI.ACPIDropTablesArray[idx].Signature && (DropTable.TableId == settingsData.ACPI.ACPIDropTablesArray[idx].TableId))) { + DropTable.MenuItem.BValue = true; + DropTable.OtherOS = settingsData.ACPI.ACPIDropTablesArray[idx].OtherOS; GlobalConfig.DropSSDT = false; // if one item=true then dropAll=false by default //DBG(" true"); Dropped = true; } - DropTable = DropTable->Next; } DBG(" %s\n", Dropped ? "yes" : "no"); } diff --git a/rEFIt_UEFI/Platform/Settings.h b/rEFIt_UEFI/Platform/Settings.h index e4452f271..2f9363240 100755 --- a/rEFIt_UEFI/Platform/Settings.h +++ b/rEFIt_UEFI/Platform/Settings.h @@ -139,19 +139,18 @@ public: class ACPI_DROP_TABLE { public: - ACPI_DROP_TABLE *Next; union { UINT32 Signature = 0; char SignatureAs4Chars[4]; }; - UINT32 Length; - UINT64 TableId; + UINT32 Length = 0; + UINT64 TableId = 0; INPUT_ITEM MenuItem = INPUT_ITEM(); - XBool OtherOS; + XBool OtherOS = false; - ACPI_DROP_TABLE() : Next(0), Signature(0), Length(0), TableId(0), OtherOS(false) {} - ACPI_DROP_TABLE(const ACPI_DROP_TABLE& other) = delete; // Can be defined if needed - const ACPI_DROP_TABLE& operator = ( const ACPI_DROP_TABLE & ) = delete; // Can be defined if needed + ACPI_DROP_TABLE() {} + ACPI_DROP_TABLE(const ACPI_DROP_TABLE& other) = default; + ACPI_DROP_TABLE& operator = ( const ACPI_DROP_TABLE & ) = default; ~ACPI_DROP_TABLE() {} }; @@ -2736,7 +2735,7 @@ public: XBool gBootChanged = false; XBool gThemeChanged = false; XBool NeedPMfix = false; - ACPI_DROP_TABLE *ACPIDropTables = NULL; + XObjArray ACPIDropTables = XObjArray(); UINT8 CustomLogoType = 0; // this will be initialized with gSettings.Boot.CustomBoot and set back to CUSTOM_BOOT_DISABLED if CustomLogo could not be loaded or decoded (see afterGetUserSettings) XImage *CustomLogo = 0; diff --git a/rEFIt_UEFI/cpp_foundation/XStringAbstract.h b/rEFIt_UEFI/cpp_foundation/XStringAbstract.h index ebdeac034..0410290ee 100644 --- a/rEFIt_UEFI/cpp_foundation/XStringAbstract.h +++ b/rEFIt_UEFI/cpp_foundation/XStringAbstract.h @@ -939,6 +939,11 @@ public: T* forgetDataWithoutFreeing() { + if ( m_allocatedSize == 0 ) { + // this is a litteral. We have to copy it before returning. If not, it'll crash when the user will free the pointer returned + if ( super::size() == 0 ) return NULL; + CheckSize(super::size()); + } T* ret = super::__m_data; super::__m_data = &nullChar; #ifdef XSTRING_CACHING_OF_SIZE @@ -1346,8 +1351,8 @@ public: super::__m_size = S->size(); #endif m_allocatedSize = S->m_allocatedSize; - // do forgetDataWithoutFreeing() last : it'll zero the value of size and m_allocatedSize - super::__m_data = S->forgetDataWithoutFreeing(); + // do not use forgetDataWithoutFreeing() : it will allocate m_data if m_data points to a literal. We want to keep the literal and avoid an allocation + super::__m_data = S->super::__m_data; return *((ThisXStringClass*)this); } diff --git a/rEFIt_UEFI/cpp_foundation/XStringArray.h b/rEFIt_UEFI/cpp_foundation/XStringArray.h index 8e01406f1..b3de6da99 100755 --- a/rEFIt_UEFI/cpp_foundation/XStringArray.h +++ b/rEFIt_UEFI/cpp_foundation/XStringArray.h @@ -213,7 +213,17 @@ public: void insertAtPos(const XStringClass1 &aString, size_t pos) { array.InsertRef(new remove_const(XStringClass1)(aString), pos, true); } template - void AddReference(CharType* newElement, bool FreeIt) { array.AddReference(new XStringClass_(newElement), FreeIt); } + void AddReference(CharType* newElement, bool FreeIt) { + if ( FreeIt ) { + XStringClass_* s = new XStringClass_(); + s->stealValueFrom(newElement); + array.AddReference(s, true); + }else{ + XStringClass_* s = new XStringClass_(); + s->takeValueFrom(newElement); + array.AddReference(s, true); + } + } void AddReference(XStringClass* newElement, bool FreeIt) { array.AddReference(newElement, FreeIt); } diff --git a/rEFIt_UEFI/libeg/XTheme.h b/rEFIt_UEFI/libeg/XTheme.h index c7c830ebd..f12b2257d 100644 --- a/rEFIt_UEFI/libeg/XTheme.h +++ b/rEFIt_UEFI/libeg/XTheme.h @@ -144,7 +144,13 @@ public: const EFI_FILE& getThemeDir() const { return *ThemeDir; } - XBool IsEmbeddedTheme(void) const { return embedded; } + XBool IsEmbeddedTheme(void) + { + if (embedded) { + ThemeDir = NULL; + } + return ThemeDir == NULL; + } //fill the theme diff --git a/rEFIt_UEFI/libeg/nanosvg.cpp b/rEFIt_UEFI/libeg/nanosvg.cpp index 42bb3a3f6..214a7313d 100755 --- a/rEFIt_UEFI/libeg/nanosvg.cpp +++ b/rEFIt_UEFI/libeg/nanosvg.cpp @@ -4693,7 +4693,7 @@ void nsvg__deleteShapes(NSVGshape* shape) nsvg__deletePaths(shape->paths); } } - nsvg__delete(shape, "nsvg__deleteShapes"_XS8); +// nsvg__delete(shape, "nsvg__deleteShapes"_XS8); // TODO : BUG, it doesn't boot anymore if we do the delete. Something is wrong. shape = snext; } } diff --git a/rEFIt_UEFI/refit/menu.cpp b/rEFIt_UEFI/refit/menu.cpp index 1dffabc26..39ff6f826 100644 --- a/rEFIt_UEFI/refit/menu.cpp +++ b/rEFIt_UEFI/refit/menu.cpp @@ -439,11 +439,10 @@ void FillInputs(XBool New) //menu for drop table - if (GlobalConfig.ACPIDropTables) { - ACPI_DROP_TABLE *DropTable = GlobalConfig.ACPIDropTables; - while (DropTable) { - DropTable->MenuItem.ItemType = BoolValue; - DropTable = DropTable->Next; + if (GlobalConfig.ACPIDropTables.notEmpty()) { + for ( size_t idx = 0 ; idx < GlobalConfig.ACPIDropTables.length() ; ++idx ) { + ACPI_DROP_TABLE& DropTable = GlobalConfig.ACPIDropTables[idx]; + DropTable.MenuItem.ItemType = BoolValue; } } @@ -1982,21 +1981,21 @@ REFIT_ABSTRACT_MENU_ENTRY* SubMenuDropTables() Entry = newREFIT_MENU_ITEM_OPTIONS(&SubScreen, ActionEnter, SCREEN_TABLES, "Tables dropping->"_XS8); - if (GlobalConfig.ACPIDropTables) { - ACPI_DROP_TABLE *DropTable = GlobalConfig.ACPIDropTables; - while (DropTable) { - CopyMem((CHAR8*)&sign, (CHAR8*)&(DropTable->Signature), 4); - CopyMem((CHAR8*)&OTID, (CHAR8*)&(DropTable->TableId), 8); + if (GlobalConfig.ACPIDropTables.notEmpty()) { + for ( size_t idx = 0 ; idx < GlobalConfig.ACPIDropTables.length() ; ++idx ) + { + ACPI_DROP_TABLE& DropTable = GlobalConfig.ACPIDropTables[idx]; + + CopyMem((CHAR8*)&sign, (CHAR8*)&(DropTable.Signature), 4); + CopyMem((CHAR8*)&OTID, (CHAR8*)&(DropTable.TableId), 8); InputBootArgs = new REFIT_INPUT_DIALOG; - InputBootArgs->Title.SWPrintf("Drop \"%4.4s\" \"%8.8s\" %d", sign, OTID, DropTable->Length); + InputBootArgs->Title.SWPrintf("Drop \"%4.4s\" \"%8.8s\" %d", sign, OTID, DropTable.Length); InputBootArgs->Row = 0xFFFF; //cursor - InputBootArgs->Item = &(DropTable->MenuItem); + InputBootArgs->Item = &(DropTable.MenuItem); InputBootArgs->AtClick = ActionEnter; InputBootArgs->AtRightClick = ActionDetails; SubScreen->AddMenuEntry(InputBootArgs, true); - - DropTable = DropTable->Next; } }