Why did I put the FreePool parameter as const. That's stupid !

You must never free a const pointer, by definition.
CloverHackyColor · Nov 11, 2023 · 187400d · 187400d
1 parent 22d277d
commit 187400d
Show file tree

Hide file tree

Showing 4 changed files with 15 additions and 8 deletions.
diff --git a/MdePkg/Include/Library/MemoryAllocationLib.h b/MdePkg/Include/Library/MemoryAllocationLib.h
@@ -478,10 +478,13 @@ ReallocateReservedPool (
   @param  Buffer                Pointer to the buffer to free.
 
 **/
+/*
+ * Jief : One day, I put JCONST. This is wrong !!! You must never free a ptr that's const.
+ */
 VOID
 EFIAPI
 FreePool(
-  IN JCONST VOID   *Buffer
+  IN VOID   *Buffer
   );
 
 #endif
diff --git a/rEFIt_UEFI/entry_scan/loader.cpp b/rEFIt_UEFI/entry_scan/loader.cpp
@@ -681,7 +681,8 @@ MacOsVersion GetOSVersion(int LoaderType, const EFI_GUID& APFSTargetUUID, const
     // Check for ia.log - InstallESD/createinstallmedia/startosinstall
     // Implemented by Sherlocks
     if (OSVersion.isEmpty()) {
-      CONST CHAR8  *s, *fileBuffer;
+      CONST CHAR8  *s;
+      UINT8* fileBuffer;
 //      CHAR8  *Res5 = (__typeof__(Res5))AllocateZeroPool(5);
 //      CHAR8  *Res6 = (__typeof__(Res6))AllocateZeroPool(6);
 //      CHAR8  *Res7 = (__typeof__(Res7))AllocateZeroPool(7);
@@ -696,10 +697,10 @@ MacOsVersion GetOSVersion(int LoaderType, const EFI_GUID& APFSTargetUUID, const
         }
       }
       if (FileExists (Volume->RootDir, InstallerLog)) {
-        Status = egLoadFile(Volume->RootDir, InstallerLog.wc_str(), (UINT8 **)&fileBuffer, &fileLen);
+        Status = egLoadFile(Volume->RootDir, InstallerLog.wc_str(), &fileBuffer, &fileLen);
         if (!EFI_ERROR(Status)) {
           XString8 targetString;
-          targetString.strncpy(fileBuffer, fileLen);
+          targetString.strncpy((CHAR8*)fileBuffer, fileLen);
       //    s = SearchString(targetString, fileLen, "Running OS Build: Mac OS X ", 27);
           s = AsciiStrStr(targetString.c_str(), "Running OS Build: Mac OS X ");
           if (s[31] == ' ') {

diff --git a/rEFIt_UEFI/libeg/XTheme.cpp b/rEFIt_UEFI/libeg/XTheme.cpp
@@ -125,8 +125,8 @@ EFI_STATUS Status       = EFI_NOT_FOUND;
         }
         TestTheme.setEmpty();
       }
-      FreePool(ChosenTheme);
-      ChosenTheme = NULL;
+      //FreePool(ChosenTheme); // ChosenTheme is an argument passed here, so the callee is "own" that memory.
+      //ChosenTheme = NULL;    // Why is this bad pratice : it's assuming that ChosenTheme was dynamically allocated and freeable. What is this the content of an XString ?
     }
     // Try to get theme from settings
     if (ThemeDict == NULL) {
@@ -204,7 +204,8 @@ EFI_STATUS Status       = EFI_NOT_FOUND;
     }
   }
   if (ChosenTheme != NULL) {
-    FreePool(ChosenTheme);
+      //FreePool(ChosenTheme); // ChosenTheme is an argument passed here, so the callee is "own" that memory.
+                               // Why is this bad pratice : it's assuming that ChosenTheme was dynamically allocated and freeable. What is this the content of an XString ?
   }
   if (!ThemeX->TypeSVG) {
     ThemeX->PrepareFont();

diff --git a/rEFIt_UEFI/refit/main.cpp b/rEFIt_UEFI/refit/main.cpp
@@ -3030,7 +3030,9 @@ RefitMain (IN EFI_HANDLE           ImageHandle,
     if (!GlobalConfig.isFastBoot()) {
       if (gThemeNeedInit) {
         UINTN      Size         = 0;
-        InitTheme((CHAR8*)GetNvramVariable(L"Clover.Theme", gEfiAppleBootGuid, NULL, &Size));
+        CHAR8* ChoosenTheme = (CHAR8*)GetNvramVariable(L"Clover.Theme", gEfiAppleBootGuid, NULL, &Size);
+        InitTheme(ChoosenTheme);
+        FreePool(ChoosenTheme);
         gThemeNeedInit = false;
       } else if (GlobalConfig.gThemeChanged) {
         DBG("change theme\n");