Clover Problems and Solutions

Zenith432 · November 18, 2017

Bug described in posts #2617, #2620 fixed in Clover r4306.

RehabMan · November 18, 2017

Hi Zenith432,

I think it will be interesting for you.

I noticed this bug many revisions ago and now I was clearly reproduced it.

1. Updated EDK2 and Clover to latest revision.

2. Compile with ./ebuilds.sh -t XCODE8

3. Compile with ./ebuilds.sh -t XCODE5

Compare results

Xcode8

screenshot11.png

Xcode5

screenshot12.png

This is same sources rev4304, same computer #1, same macOS 10.12.6, and same XCode 9.1.

The results are different by different toolset.

Affected only embedded theme.

Any thought?

I'll look into it.

I've been doing some investigation on this...

My current guess: stack overflow causing heap corruption? (AllocatePool is failing for a 64kb block for the icon images)

If someone understands how stack is allocated/sized in this scenario, maybe there are some linker options that can be changed for experimentation.

Some diffs I've been experimenting with:

diff --git a/rEFIt_UEFI/libeg/image.c b/rEFIt_UEFI/libeg/image.c
index a7220404..ab173240 100644
--- a/rEFIt_UEFI/libeg/image.c
+++ b/rEFIt_UEFI/libeg/image.c
@@ -54,6 +54,8 @@
 #define DBG(...) DebugLog(DEBUG_IMG, __VA_ARGS__)
 #endif
 
+#define RM_DEBUG
+
 //
 // Basic image handling
 //
@@ -61,12 +63,21 @@
 EG_IMAGE * egCreateImage(IN INTN Width, IN INTN Height, IN BOOLEAN HasAlpha)
 {
     EG_IMAGE        *NewImage;
-
+#ifdef RM_DEBUG
+    DBG("egCreateImage(%d,%d,%d) entered\n", Width, Height, (INTN)HasAlpha);
+#endif
     NewImage = (EG_IMAGE *) AllocatePool(sizeof(EG_IMAGE));
-    if (NewImage == NULL)
+    if (NewImage == NULL) {
+#ifdef RM_DEBUG
+        DBG("egCreateImage AllocatePool for EG_IMAGE failed\n");
+#endif
         return NULL;
+    }
     NewImage->PixelData = (EG_PIXEL *) AllocatePool((UINTN)(Width * Height * sizeof(EG_PIXEL)));
     if (NewImage->PixelData == NULL) {
+#ifdef RM_DEBUG
+        DBG("egCreateImage AllocatePool(%d) for PixelData failed\n", (UINTN)(Width * Height * sizeof(EG_PIXEL)));
+#endif
         FreePool(NewImage);
         return NULL;
     }
@@ -325,12 +336,25 @@ EFI_STATUS egLoadFile(IN EFI_FILE_HANDLE BaseDir, IN CHAR16 *FileName,
     UINTN               BufferSize;
     UINT8               *Buffer;
 
+#ifdef RM_DEBUG
+    DBG("egLoadFile(\"%s\"...", FileName);
+#endif
+    *FileData = NULL;
+    *FileDataLength = 0;
+
     Status = BaseDir->Open(BaseDir, &FileHandle, FileName, EFI_FILE_MODE_READ, 0);
-    if (EFI_ERROR(Status))
+    if (EFI_ERROR(Status)) {
+#ifdef RM_DEBUG
+        DBG("Open failed\n");
+#endif
         return Status;
+    }
 
     FileInfo = EfiLibFileInfo(FileHandle);
     if (FileInfo == NULL) {
+#ifdef RM_DEBUG
+        DBG("not found\n");
+#endif
         FileHandle->Close(FileHandle);
         return EFI_NOT_FOUND;
     }
@@ -342,6 +366,9 @@ EFI_STATUS egLoadFile(IN EFI_FILE_HANDLE BaseDir, IN CHAR16 *FileName,
     BufferSize = (UINTN)ReadSize;   // was limited to 1 GB above, so this is safe
     Buffer = (UINT8 *) AllocatePool (BufferSize);
     if (Buffer == NULL) {
+#ifdef RM_DEBUG
+        DBG("AllocatePool failed\n");
+#endif
         FileHandle->Close(FileHandle);
         return EFI_OUT_OF_RESOURCES;
     }
@@ -349,10 +376,16 @@ EFI_STATUS egLoadFile(IN EFI_FILE_HANDLE BaseDir, IN CHAR16 *FileName,
     Status = FileHandle->Read(FileHandle, &BufferSize, Buffer);
     FileHandle->Close(FileHandle);
     if (EFI_ERROR(Status)) {
+#ifdef RM_DEBUG
+        DBG("Read failed\n");
+#endif
         FreePool(Buffer);
         return Status;
     }
 
+#ifdef RM_DEBUG
+    DBG("Success, length=%d\n", BufferSize);
+#endif
     *FileData = Buffer;
     *FileDataLength = BufferSize;
     return EFI_SUCCESS;
@@ -584,7 +617,7 @@ EG_IMAGE * egLoadIcon(IN EFI_FILE_HANDLE BaseDir, IN CHAR16 *FileName, IN UINTN
   // decode it
   NewImage = egDecodePNG(FileData, FileDataLength, TRUE);
   if (!NewImage) {
-//    DBG("not png, try icns\n");
+    DBG("not png, try icns\n");
     NewImage = egDecodeICNS(FileData, FileDataLength, IconSize, TRUE);
   }
   
@@ -707,6 +740,7 @@ VOID egRestrictImageArea(IN EG_IMAGE *Image,
 
 VOID egFillImage(IN OUT EG_IMAGE *CompImage, IN EG_PIXEL *Color)
 {
+#if 0
   INTN       i;
   EG_PIXEL    FillColor;
   EG_PIXEL    *PixelPtr;
@@ -722,6 +756,19 @@ VOID egFillImage(IN OUT EG_IMAGE *CompImage, IN EG_PIXEL *Color)
   for (i = 0; i < CompImage->Width * CompImage->Height; i++) {
     *PixelPtr++ = FillColor;
   }
+#else
+  if (!CompImage || !Color)
+    return;
+
+  EG_PIXEL FillColor = *Color;
+  if (!CompImage->HasAlpha)
+    FillColor.a = 0;
+
+  EG_PIXEL* PixelPtr = CompImage->PixelData;
+  EG_PIXEL* PixelEnd = CompImage->PixelData + CompImage->Width * CompImage->Height;
+  while (PixelPtr < PixelEnd)
+    *PixelPtr++ = FillColor;
+#endif
 }
 
 VOID egFillImageArea(IN OUT EG_IMAGE *CompImage,
diff --git a/rEFIt_UEFI/refit/icns.c b/rEFIt_UEFI/refit/icns.c
index 2fd8608f..02d7d81a 100644
--- a/rEFIt_UEFI/refit/icns.c
+++ b/rEFIt_UEFI/refit/icns.c
@@ -49,6 +49,8 @@
 #define DBG(...) DebugLog(DEBUG_ICNS, __VA_ARGS__)
 #endif
 
+#define RM_DEBUG
+
 //
 // well-known icons
 //
@@ -207,54 +209,71 @@ EG_IMAGE * BuiltinIcon(IN UINTN Id)
 EG_IMAGE * LoadOSIcon(IN CHAR16 *OSIconName OPTIONAL, IN CHAR16 *FallbackIconName, IN UINTN PixelSize, IN BOOLEAN BootLogo, IN BOOLEAN WantDummy)
 {
   EG_IMAGE        *Image;
-  CHAR16          CutoutName[16], FirstName[16];
-  CHAR16          FileName[256];
-  UINTN           StartIndex, Index, NextIndex;
-
+  CHAR16          CutoutName[16];
+  //CHAR16          FirstName[sizeof(CutoutName)/sizeof(CutoutName[0])];
+  //CHAR16          FileName[256];
+  CHAR16*         FileName;
+  UINTN           Index, NextIndex;
+#ifdef RM_DEBUG
+  DBG("LoadOSIcon(\"%s\",\"%s\",%d,%d,%d)\n", OSIconName, FallbackIconName, (int)PixelSize, (int)BootLogo, (int)WantDummy);
+#endif
   if (GlobalConfig.TextOnly)      // skip loading if it's not used anyway
     return NULL;
   Image = NULL;
-  *FirstName = 0;
-
-  // try the names from OSIconName
-  for (StartIndex = 0; OSIconName != NULL && OSIconName[StartIndex]; StartIndex = NextIndex) {
-    // find the next name in the list
-    NextIndex = 0;
-    for (Index = StartIndex; OSIconName[Index]; Index++) {
-      if (OSIconName[Index] == ',') {
-        NextIndex = Index + 1;
-        break;
+  //FirstName[0] = 0;
+
+  if (OSIconName) {
+    // try the names from OSIconName
+    for (Index = 0; OSIconName[Index]; Index = NextIndex) {
+      // find the delimiter
+      for (NextIndex = Index; OSIconName[NextIndex] && OSIconName[NextIndex] != ','; NextIndex++)
+        ;
+#ifdef RM_DEBUG
+      DBG("NextIndex=%d, Index=%d\n", NextIndex, Index);
+#endif
+      // construct full path
+      UINTN CutoutLen = NextIndex-Index;
+      if (CutoutLen < sizeof(CutoutName)/sizeof(CutoutName[0])) { // prevent buffer overflow
+        CopyMem(CutoutName, OSIconName + Index, CutoutLen * sizeof(CHAR16));
+        CutoutName[CutoutLen] = 0;
+#ifdef RM_DEBUG
+        DBG("CutoutName=\"%s\", NextIndex=%d, Index=%d\n", CutoutName, NextIndex, Index);
+#endif
+        //UnicodeSPrint(FileName, sizeof(FileName), L"icons\\%s_%s.icns",
+                      //BootLogo ? L"boot" : L"os", CutoutName);
+        FileName = PoolPrint(L"icons\\%s_%s.icns", BootLogo ? L"boot" : L"os", CutoutName);
+#ifdef RM_DEBUG
+        DBG("FileName = \"%s\"\n", FileName);
+#endif
+        // try to load it
+        Image = egLoadIcon(ThemeDir, FileName, PixelSize);
+        FreePool(FileName);
+        if (Image != NULL) {
+          return Image;
+        }
+        //if (0 == FirstName[0]) {
+        //  //CopyMem(FirstName, CutoutName, StrSize(CutoutName));
+        //  StrCpy(FirstName, CutoutName);
+#ifdef RM_DEBUG
+        //  DBG("CutoutName size = %d\n", StrSize(CutoutName));
+#endif
+        //  if ('a' <= FirstName[0] && FirstName[0] <= 'z') {
+        //    FirstName[0] = (CHAR16) (FirstName[0] - 0x20);
+        //  }
+        //}
       }
-    }
-    if (OSIconName[Index] == 0)
-      NextIndex = Index;
-
-    // construct full path
-    if (Index > StartIndex + 15)   // prevent buffer overflow
-      continue;
-    CopyMem(CutoutName, OSIconName + StartIndex, (Index - StartIndex) * sizeof(CHAR16));
-    CutoutName[Index - StartIndex] = 0;
-    UnicodeSPrint(FileName, 512, L"icons\\%s_%s.icns",
-                  BootLogo ? L"boot" : L"os", CutoutName);
-
-    // try to load it
-    Image = egLoadIcon(ThemeDir, FileName, PixelSize);
-    if (Image != NULL) {
-      return Image;
-    }
-
-    if (*FirstName == '\0') {
-      CopyMem(FirstName, CutoutName, StrSize(CutoutName));
-      if ('a' <= FirstName[0] && FirstName[0] <= 'z') {
-        FirstName[0] = (CHAR16) (FirstName[0] - 0x20);
+      if (',' == OSIconName[NextIndex]) {
+        NextIndex++;
       }
     }
   }
 
   // try the fallback name
-  UnicodeSPrint(FileName, 512, L"icons\\%s_%s.icns",
-                BootLogo ? L"boot" : L"os", FallbackIconName);
+  //UnicodeSPrint(FileName, sizeof(FileName), L"icons\\%s_%s.icns",
+                //BootLogo ? L"boot" : L"os", FallbackIconName);
+  FileName = PoolPrint(L"icons\\%s_%s.icns", BootLogo ? L"boot" : L"os", FallbackIconName);
   Image = egLoadIcon(ThemeDir, FileName, PixelSize);
+  FreePool(FileName);
   if (Image != NULL) {
     return Image;
   }
@@ -271,10 +290,13 @@ EG_IMAGE * LoadOSIcon(IN CHAR16 *OSIconName OPTIONAL, IN CHAR16 *FallbackIconNam
   }
 
   if (IsEmbeddedTheme()) { // embedded theme - return rendered icon name
+#ifdef RM_DEBUG
+    DBG("creating embedded/filled image\n");
+#endif
     EG_IMAGE  *TextBuffer = egCreateImage(PixelSize, PixelSize, TRUE);
     egFillImage(TextBuffer, &MenuBackgroundPixel);
-//    egRenderText(FirstName, TextBuffer, PixelSize/4, PixelSize/3, 0xFFFF);
-//    DebugLog(1, "Text <%s> rendered\n", FirstName);
+    //egRenderText(FirstName, TextBuffer, PixelSize/4, PixelSize/3, 0xFFFF);
+    //DebugLog(1, "Text <%s> rendered\n", FirstName);
     return TextBuffer;
   }

The rewrite regarding FirstName access (which is an auto that touched but is never really used in the current code) seems to improve things somewhat, which is what makes me think it is stack related.

ErmaC · November 18, 2017

Is it possible to change permission on a thread/topic to only allow coders, developers and the various forms of moderators/admin? Because I like the idea of discussion threads for random clover talk, clover bug/issue/request reports, and clover development (which is only locked to developers). I think that most users see this as a place to report problems or request feaures, which because of the thread title seems correct. So either we should create a separate developer only thread about development, or rename this thread to development only and create another separate bug/issue/request thread. No?

As a second (and last) step relate to this request I open a topic here -> Clover problems report & features request for all the user where they are free to report bug or feature request

Cordially

ErmaC

apianti · November 19, 2017

I've been doing some investigation on this...

My current guess: stack overflow causing heap corruption? (AllocatePool is failing for a 64kb block for the icon images)

If someone understands how stack is allocated/sized in this scenario, maybe there are some linker options that can be changed for experimentation.

Some diffs I've been experimenting with:

diff --git a/rEFIt_UEFI/libeg/image.c b/rEFIt_UEFI/libeg/image.c
index a7220404..ab173240 100644
--- a/rEFIt_UEFI/libeg/image.c
+++ b/rEFIt_UEFI/libeg/image.c
@@ -54,6 +54,8 @@
 #define DBG(...) DebugLog(DEBUG_IMG, __VA_ARGS__)
 #endif
 
+#define RM_DEBUG
+
 //
 // Basic image handling
 //
@@ -61,12 +63,21 @@
 EG_IMAGE * egCreateImage(IN INTN Width, IN INTN Height, IN BOOLEAN HasAlpha)
 {
     EG_IMAGE        *NewImage;
-
+#ifdef RM_DEBUG
+    DBG("egCreateImage(%d,%d,%d) entered\n", Width, Height, (INTN)HasAlpha);
+#endif
     NewImage = (EG_IMAGE *) AllocatePool(sizeof(EG_IMAGE));
-    if (NewImage == NULL)
+    if (NewImage == NULL) {
+#ifdef RM_DEBUG
+        DBG("egCreateImage AllocatePool for EG_IMAGE failed\n");
+#endif
         return NULL;
+    }
     NewImage->PixelData = (EG_PIXEL *) AllocatePool((UINTN)(Width * Height * sizeof(EG_PIXEL)));
     if (NewImage->PixelData == NULL) {
+#ifdef RM_DEBUG
+        DBG("egCreateImage AllocatePool(%d) for PixelData failed\n", (UINTN)(Width * Height * sizeof(EG_PIXEL)));
+#endif
         FreePool(NewImage);
         return NULL;
     }
@@ -325,12 +336,25 @@ EFI_STATUS egLoadFile(IN EFI_FILE_HANDLE BaseDir, IN CHAR16 *FileName,
     UINTN               BufferSize;
     UINT8               *Buffer;
 
+#ifdef RM_DEBUG
+    DBG("egLoadFile(\"%s\"...", FileName);
+#endif
+    *FileData = NULL;
+    *FileDataLength = 0;
+
     Status = BaseDir->Open(BaseDir, &FileHandle, FileName, EFI_FILE_MODE_READ, 0);
-    if (EFI_ERROR(Status))
+    if (EFI_ERROR(Status)) {
+#ifdef RM_DEBUG
+        DBG("Open failed\n");
+#endif
         return Status;
+    }
 
     FileInfo = EfiLibFileInfo(FileHandle);
     if (FileInfo == NULL) {
+#ifdef RM_DEBUG
+        DBG("not found\n");
+#endif
         FileHandle->Close(FileHandle);
         return EFI_NOT_FOUND;
     }
@@ -342,6 +366,9 @@ EFI_STATUS egLoadFile(IN EFI_FILE_HANDLE BaseDir, IN CHAR16 *FileName,
     BufferSize = (UINTN)ReadSize;   // was limited to 1 GB above, so this is safe
     Buffer = (UINT8 *) AllocatePool (BufferSize);
     if (Buffer == NULL) {
+#ifdef RM_DEBUG
+        DBG("AllocatePool failed\n");
+#endif
         FileHandle->Close(FileHandle);
         return EFI_OUT_OF_RESOURCES;
     }
@@ -349,10 +376,16 @@ EFI_STATUS egLoadFile(IN EFI_FILE_HANDLE BaseDir, IN CHAR16 *FileName,
     Status = FileHandle->Read(FileHandle, &BufferSize, Buffer);
     FileHandle->Close(FileHandle);
     if (EFI_ERROR(Status)) {
+#ifdef RM_DEBUG
+        DBG("Read failed\n");
+#endif
         FreePool(Buffer);
         return Status;
     }
 
+#ifdef RM_DEBUG
+    DBG("Success, length=%d\n", BufferSize);
+#endif
     *FileData = Buffer;
     *FileDataLength = BufferSize;
     return EFI_SUCCESS;
@@ -584,7 +617,7 @@ EG_IMAGE * egLoadIcon(IN EFI_FILE_HANDLE BaseDir, IN CHAR16 *FileName, IN UINTN
   // decode it
   NewImage = egDecodePNG(FileData, FileDataLength, TRUE);
   if (!NewImage) {
-//    DBG("not png, try icns\n");
+    DBG("not png, try icns\n");
     NewImage = egDecodeICNS(FileData, FileDataLength, IconSize, TRUE);
   }
   
@@ -707,6 +740,7 @@ VOID egRestrictImageArea(IN EG_IMAGE *Image,
 
 VOID egFillImage(IN OUT EG_IMAGE *CompImage, IN EG_PIXEL *Color)
 {
+#if 0
   INTN       i;
   EG_PIXEL    FillColor;
   EG_PIXEL    *PixelPtr;
@@ -722,6 +756,19 @@ VOID egFillImage(IN OUT EG_IMAGE *CompImage, IN EG_PIXEL *Color)
   for (i = 0; i < CompImage->Width * CompImage->Height; i++) {
     *PixelPtr++ = FillColor;
   }
+#else
+  if (!CompImage || !Color)
+    return;
+
+  EG_PIXEL FillColor = *Color;
+  if (!CompImage->HasAlpha)
+    FillColor.a = 0;
+
+  EG_PIXEL* PixelPtr = CompImage->PixelData;
+  EG_PIXEL* PixelEnd = CompImage->PixelData + CompImage->Width * CompImage->Height;
+  while (PixelPtr < PixelEnd)
+    *PixelPtr++ = FillColor;
+#endif
 }
 
 VOID egFillImageArea(IN OUT EG_IMAGE *CompImage,
diff --git a/rEFIt_UEFI/refit/icns.c b/rEFIt_UEFI/refit/icns.c
index 2fd8608f..02d7d81a 100644
--- a/rEFIt_UEFI/refit/icns.c
+++ b/rEFIt_UEFI/refit/icns.c
@@ -49,6 +49,8 @@
 #define DBG(...) DebugLog(DEBUG_ICNS, __VA_ARGS__)
 #endif
 
+#define RM_DEBUG
+
 //
 // well-known icons
 //
@@ -207,54 +209,71 @@ EG_IMAGE * BuiltinIcon(IN UINTN Id)
 EG_IMAGE * LoadOSIcon(IN CHAR16 *OSIconName OPTIONAL, IN CHAR16 *FallbackIconName, IN UINTN PixelSize, IN BOOLEAN BootLogo, IN BOOLEAN WantDummy)
 {
   EG_IMAGE        *Image;
-  CHAR16          CutoutName[16], FirstName[16];
-  CHAR16          FileName[256];
-  UINTN           StartIndex, Index, NextIndex;
-
+  CHAR16          CutoutName[16];
+  //CHAR16          FirstName[sizeof(CutoutName)/sizeof(CutoutName[0])];
+  //CHAR16          FileName[256];
+  CHAR16*         FileName;
+  UINTN           Index, NextIndex;
+#ifdef RM_DEBUG
+  DBG("LoadOSIcon(\"%s\",\"%s\",%d,%d,%d)\n", OSIconName, FallbackIconName, (int)PixelSize, (int)BootLogo, (int)WantDummy);
+#endif
   if (GlobalConfig.TextOnly)      // skip loading if it's not used anyway
     return NULL;
   Image = NULL;
-  *FirstName = 0;
-
-  // try the names from OSIconName
-  for (StartIndex = 0; OSIconName != NULL && OSIconName[StartIndex]; StartIndex = NextIndex) {
-    // find the next name in the list
-    NextIndex = 0;
-    for (Index = StartIndex; OSIconName[Index]; Index++) {
-      if (OSIconName[Index] == ',') {
-        NextIndex = Index + 1;
-        break;
+  //FirstName[0] = 0;
+
+  if (OSIconName) {
+    // try the names from OSIconName
+    for (Index = 0; OSIconName[Index]; Index = NextIndex) {
+      // find the delimiter
+      for (NextIndex = Index; OSIconName[NextIndex] && OSIconName[NextIndex] != ','; NextIndex++)
+        ;
+#ifdef RM_DEBUG
+      DBG("NextIndex=%d, Index=%d\n", NextIndex, Index);
+#endif
+      // construct full path
+      UINTN CutoutLen = NextIndex-Index;
+      if (CutoutLen < sizeof(CutoutName)/sizeof(CutoutName[0])) { // prevent buffer overflow
+        CopyMem(CutoutName, OSIconName + Index, CutoutLen * sizeof(CHAR16));
+        CutoutName[CutoutLen] = 0;
+#ifdef RM_DEBUG
+        DBG("CutoutName=\"%s\", NextIndex=%d, Index=%d\n", CutoutName, NextIndex, Index);
+#endif
+        //UnicodeSPrint(FileName, sizeof(FileName), L"icons\\%s_%s.icns",
+                      //BootLogo ? L"boot" : L"os", CutoutName);
+        FileName = PoolPrint(L"icons\\%s_%s.icns", BootLogo ? L"boot" : L"os", CutoutName);
+#ifdef RM_DEBUG
+        DBG("FileName = \"%s\"\n", FileName);
+#endif
+        // try to load it
+        Image = egLoadIcon(ThemeDir, FileName, PixelSize);
+        FreePool(FileName);
+        if (Image != NULL) {
+          return Image;
+        }
+        //if (0 == FirstName[0]) {
+        //  //CopyMem(FirstName, CutoutName, StrSize(CutoutName));
+        //  StrCpy(FirstName, CutoutName);
+#ifdef RM_DEBUG
+        //  DBG("CutoutName size = %d\n", StrSize(CutoutName));
+#endif
+        //  if ('a' <= FirstName[0] && FirstName[0] <= 'z') {
+        //    FirstName[0] = (CHAR16) (FirstName[0] - 0x20);
+        //  }
+        //}
       }
-    }
-    if (OSIconName[Index] == 0)
-      NextIndex = Index;
-
-    // construct full path
-    if (Index > StartIndex + 15)   // prevent buffer overflow
-      continue;
-    CopyMem(CutoutName, OSIconName + StartIndex, (Index - StartIndex) * sizeof(CHAR16));
-    CutoutName[Index - StartIndex] = 0;
-    UnicodeSPrint(FileName, 512, L"icons\\%s_%s.icns",
-                  BootLogo ? L"boot" : L"os", CutoutName);
-
-    // try to load it
-    Image = egLoadIcon(ThemeDir, FileName, PixelSize);
-    if (Image != NULL) {
-      return Image;
-    }
-
-    if (*FirstName == '\0') {
-      CopyMem(FirstName, CutoutName, StrSize(CutoutName));
-      if ('a' <= FirstName[0] && FirstName[0] <= 'z') {
-        FirstName[0] = (CHAR16) (FirstName[0] - 0x20);
+      if (',' == OSIconName[NextIndex]) {
+        NextIndex++;
       }
     }
   }
 
   // try the fallback name
-  UnicodeSPrint(FileName, 512, L"icons\\%s_%s.icns",
-                BootLogo ? L"boot" : L"os", FallbackIconName);
+  //UnicodeSPrint(FileName, sizeof(FileName), L"icons\\%s_%s.icns",
+                //BootLogo ? L"boot" : L"os", FallbackIconName);
+  FileName = PoolPrint(L"icons\\%s_%s.icns", BootLogo ? L"boot" : L"os", FallbackIconName);
   Image = egLoadIcon(ThemeDir, FileName, PixelSize);
+  FreePool(FileName);
   if (Image != NULL) {
     return Image;
   }
@@ -271,10 +290,13 @@ EG_IMAGE * LoadOSIcon(IN CHAR16 *OSIconName OPTIONAL, IN CHAR16 *FallbackIconNam
   }
 
   if (IsEmbeddedTheme()) { // embedded theme - return rendered icon name
+#ifdef RM_DEBUG
+    DBG("creating embedded/filled image\n");
+#endif
     EG_IMAGE  *TextBuffer = egCreateImage(PixelSize, PixelSize, TRUE);
     egFillImage(TextBuffer, &MenuBackgroundPixel);
-//    egRenderText(FirstName, TextBuffer, PixelSize/4, PixelSize/3, 0xFFFF);
-//    DebugLog(1, "Text <%s> rendered\n", FirstName);
+    //egRenderText(FirstName, TextBuffer, PixelSize/4, PixelSize/3, 0xFFFF);
+    //DebugLog(1, "Text <%s> rendered\n", FirstName);
     return TextBuffer;
   }

The rewrite regarding FirstName access (which is an auto that touched but is never really used in the current code) seems to improve things somewhat, which is what makes me think it is stack related.

If this was being caused by a stack overflow then you would most likely see artifacts instead of nothing or failing to allocate. It's probably just being built incorrectly. Also auto variables are created on the stack, so you creating another stack variable should have caused more problems, not less. So I doubt it's stack problem, but it seems Zenith432 fixed it anyway.

Zenith432 · November 19, 2017

The bug was that eglodepng_decode was storing 32-bit unsigned ints in the lower 32-bit of 64-bit unsigned ints. egDecodePNG was then using the entire 64-bit value. If the upper 32-bit happened to be non-zero, it would ask egCreateImage to allocate a huge amount of memory (> 4GB). There was no memory corruption, just safely failed allocation and the icons would not display. The bug is not compiler-dependent, but happened to show up in specific scenarios.

@RehabMan: you might want to switch to using %ld when printing INTN.

RehabMan · November 19, 2017

The bug was that eglodepng_decode was storing 32-bit unsigned ints in the lower 32-bit of 64-bit unsigned ints. egDecodePNG was then using the entire 64-bit value. If the upper 32-bit happened to be non-zero, it would ask egCreateImage to allocate a huge amount of memory (> 4GB). There was no memory corruption, just safely failed allocation and the icons would not display. The bug is not compiler-dependent, but happened to show up in specific scenarios.

So it was essentially the same as an uninitialized auto, which was the other possibility going through my head.

Which explains why it was affected by what was on the stack previously.

On a large project I worked on (somewhat distant past), we used a custom stack probe in the debug builds that initialized all autos to non-zero (I forget the specific pattern we used), which allowed easy/consistent detection of such bugs.

The other lesson here is that type-casts (especially with pointer types) should not be taken lightly.

@RehabMan: you might want to switch to using %ld when printing INTN.

Yes... My misunderstanding regarding INTN which I read as 'natural int', which one would assume is 'int'.

But it isn't (it is either INT32 (aka. int) or INT64 (aka. long long), depending build target/pointer size).

Probably best to do all debugging with %lld and cast to INT64... I would have seen the bug so obviously then...

RehabMan · November 19, 2017

Rev 4307

cleanup, fix and support more CPU for Generate option

process for power management in old/new Clover

-for sandy-, default - disabled P/C States generate option

-for ivy+, default - enabled P/C States generate option

Do not mix the custom power management file(SSDT.aml) and the P/C States option

if you want to use power management

1. only use P/C States option without SSDT.aml

2. turn off P/C States option. then put SSDT.aml in ACPI/patched folder
		<key>SSDT</key>
		<dict>
			<key>Generate</key>
			<dict>
				<key>CStates</key>
				<false/>
				<key>PStates</key>
				<false/>
			</dict>
		</dict>

The changes to Settings.c break backward compatibility.

It should be reverted.

It is important that the new "Generate" features (APSN, APLF, PluginType) follow the setting for Generate/PStates when they (the new options) are not specified. It would be different (probably), if all the features had been introduced as they are now (separate) initially. But instead we must deal with old config.plist files that are unaware of the new options and provide backward compatible behavior even though there are new options available.

Sherlocks · November 20, 2017

The changes to Settings.c break backward compatibility.

It should be reverted.

It is important that the new "Generate" features (APSN, APLF, PluginType) follow the setting for Generate/PStates when they (the new options) are not specified. It would be different (probably), if all the features had been introduced as they are now (separate) initially. But instead we must deal with old config.plist files that are unaware of the new options and provide backward compatible behavior even though there are new options available.

If there is no part SSDT in config.plist, APSN/APLF part not operate in Stategenerator. Some user has not SSDT in config.plist. I tested all keys according to true/false and without SSDT key for all cases on my Skylake/Sandy laptop. There is no problem. And you can check it from log.

Ivy+, default enabled P/C states. But sandy- is no.

Also APSN/APLF keys according to ~~C states in setting if there is C states.~~

EDIT1

C states in setting if there is C states.

-> P states

나의 LG-F800S 의 Tapatalk에서 보냄

Slice · November 20, 2017

Bug described in posts #2617, #2620 fixed in Clover r4306.

Not fully. https://applelife.ru/threads/clover.42089/page-871#post-694416 Hope 4308 will be better.

loadpng.c is bad desinged but why this bug affected only embedded theme?

Zenith432 · November 20, 2017

@Slice, I tested on r4308, built from Clover svn, edk2 latest from edk2 at github, patches applied from Clover svn, Xcode 9.1, ./ebuild -t XCODE8, Theme embedded, and the problem doesn't reproduce. Please try reproducing yourself, and if you can't, please give them your build to try with. I can't be sure whether what they're talking about was misbuilt or not.

Before the fix, I was able to reproduce the problem and I'm sure what I said in post #2625 was the reason.

Slice · November 20, 2017

Also APSN/APLF keys according to C states in setting if there is C states.

They are related to P-states

APSN - up to turbo states

APLF - Low Frequency states

@Slice, I tested on r4308, built from Clover svn, edk2 latest from edk2 at github, Xcode 9.1, ./ebuild -t XCODE8, Theme embedded, and the problem doesn't reproduce. Please try reproducing yourself, and if you can't, please give them your build to try with. I can't be sure whether what they're talking about was misbuilt or not.

I will test today evening.

The bug is very old, since loadpng.c was implemented and depends on compilation so yes I should see by myself compared to previous result.

Sherlocks · November 20, 2017

They are related to P-states

APSN - up to turbo states

APLF - Low Frequency states

Sorry. You are right. This is my mistake that i mentioned c-states.

Slice

I checked pike's ssdt file on each intel generation.

His script makes APLF =0 on ivy+

Is it right? i'm not sure this.

https://sourceforge.net/p/cloverefiboot/code/4307/tree//rEFIt_UEFI/Platform/StateGenerator.c#l90

i just followed pike's ssdt result.

나의 LG-F800S 의 Tapatalk에서 보냄

Slice · November 20, 2017

Sorry. You are right. This is my mistake that i mentioned c-states.

Slice

I checked pike's ssdt file on each intel generation.

His script makes APLF =0 on ivy+

Is it right? i'm not sure this.

https://sourceforge.net/p/cloverefiboot/code/4307/tree//rEFIt_UEFI/Platform/StateGenerator.c#l90

i just followed pike's ssdt result.

나의 LG-F800S 의 Tapatalk에서 보냄

Our investigations in 2013 told for Ivy Bridge

  if (gMobile) Aplf = 4;
  for (i = 0; i<47; i++) {
    //ultra-mobile
    if ((gCPUStructure.BrandString[i] != 'P') &&
        (gCPUStructure.BrandString[i+1] == 'U')) {
      Aplf = 0;
      break;
    }
  }

and zero for Haswell.

I don't know who decided what is good and what is wrong.

Sherlocks · November 20, 2017

Our investigations in 2013 told for Ivy Bridge
  if (gMobile) Aplf = 4;
  for (i = 0; i<47; i++) {
    //ultra-mobile
    if ((gCPUStructure.BrandString[i] != 'P') &&
        (gCPUStructure.BrandString[i+1] == 'U')) {
      Aplf = 0;
      break;
    }
  }
and zero for Haswell.

I don't know who decided what is good and what is wrong.

i checked real mac dump.

it's right as APLF = 0. seems current code is no problem.

EDIT1.

Name (APSN, 0x04)

Name (APLF, 0x08)

desktop cpu has APLF value. i will research APLF value from real mac for usage.

old/current clover surely not divide desktop and laptop for APLF.

Zenith432 · November 20, 2017

@Slice: I added log messages around calls to lodepng from Clover in r4309, so if icons disappear, there will be messages in debug.log if the failure is from lodepng.

Slice · November 20, 2017

Clover 4309/XCODE8

Looks like it is not loadpng.c problem.

3:714  0:003  Icon 17 decoded, pointer 7D9D3A18
3:726  0:012  - [05]: 'FREEDOS'
3:732  0:005  Scanning legacy ...
3:735  0:002   5: 'FREEDOS' (freedos) add legacy
3:739  0:004  Icon 12 decoded, pointer 7D817418
3:740  0:001   added 'Boot FreeDOS from FREEDOS' OSType=3 Icon=freedos
3:741  0:001  === [ AddCustomTool ] =====================================
3:777  0:035  Icon 8 decoded, pointer 7D817298
3:779  0:001  found tool \EFI\CLOVER\tools\Shell64U.efi
3:780  0:000  Checking EFI partition Volume 3 for Clover
3:781  0:000   Found Clover
3:782  0:001  Icon 2 decoded, pointer 7D9C3518
3:784  0:001  Icon 1 decoded, pointer 7D60F818
3:786  0:002  Icon 0 decoded, pointer 7D60F798
3:788  0:001  Icon 5 decoded, pointer 7D60F718
3:789  0:001  Icon 6 decoded, pointer 7D60F698

XCODE5 has no problem

Zenith432 · November 20, 2017

@Slice, this is a completely different problem and not related to the one you posted in #2617. The icons in these new photos are fine, and now I look again in the post in apple life.ru #2629, and I see the icons there are fine too. In your post #2617 there is no black bar.

Slice · November 20, 2017

XCODE8 compilation

                                       ; Basic Block Input Regs: rax rcx -  Killed Regs: rbx r14
                                            _egCreateFilledImage:
000000000000005c 55                              push       rbp                           ; XREF=0x136b
000000000000005d 4889E5                          mov        rbp, rsp
0000000000000060 4156                            push       r14
0000000000000062 53                              push       rbx
0000000000000063 4989CE                          mov        r14, rcx
0000000000000066 E800000000                      call       0x6b      //egCreateImage
000000000000006b 4889C3                          mov        rbx, rax                      ; XREF=0x66
000000000000006e 4885DB                          test       rbx, rbx
0000000000000071 740D                            je         0x80
                                       ; Basic Block Input Regs: rbx r14 -  Killed Regs: rsi rdi
0000000000000073 4889DF                          mov        rdi, rbx
0000000000000076 4C89F6                          mov        rsi, r14
0000000000000079 E800000000                      call       0x7e     //egFillImage
000000000000007e EB02                            jmp        0x82                          ; XREF=0x79
                                       ; Basic Block Input Regs: rbx -  Killed Regs: rbx
0000000000000080 31DB                            xor        ebx, ebx                      ; XREF=0x71
                                       ; Basic Block Input Regs: rbx rsp -  Killed Regs: rax rbx rbp r14
0000000000000082 4889D8                          mov        rax, rbx                      ; XREF=0x7e
0000000000000085 5B                              pop        rbx
0000000000000086 415E                            pop        r14
0000000000000088 5D                              pop        rbp
0000000000000089 C3                              ret        
                        ; endp

and XCODE5 compilation

                                      ; Basic Block Input Regs: rax r9 -  Killed Regs: rsi rdi
                                            _egCreateFilledImage:
0000000000000066 55                              push       rbp
0000000000000067 4889E5                          mov        rbp, rsp
000000000000006a 56                              push       rsi
000000000000006b 57                              push       rdi
000000000000006c 4883EC20                        sub        rsp, 0x20
0000000000000070 4C89CE                          mov        rsi, r9
0000000000000073 E800000000                      call       0x78
0000000000000078 4889C7                          mov        rdi, rax                      ; XREF=0x73
000000000000007b 4885FF                          test       rdi, rdi
000000000000007e 740D                            je         0x8d
                                       ; Basic Block Input Regs: rsi rdi -  Killed Regs: rcx rdx
0000000000000080 4889F9                          mov        rcx, rdi
0000000000000083 4889F2                          mov        rdx, rsi
0000000000000086 E800000000                      call       0x8b
000000000000008b EB02                            jmp        0x8f                          ; XREF=0x86
                                       ; Basic Block Input Regs: rdi -  Killed Regs: rdi
000000000000008d 31FF                            xor        edi, edi                      ; XREF=0x7e
                                       ; Basic Block Input Regs: rdi -  Killed Regs: rax rsp rbp rsi rdi
000000000000008f 4889F8                          mov        rax, rdi                      ; XREF=0x8b
0000000000000092 4883C420                        add        rsp, 0x20
0000000000000096 5F                              pop        rdi
0000000000000097 5E                              pop        rsi
0000000000000098 5D                              pop        rbp
0000000000000099 C3                              ret        
                        ; endp

@Slice, this is a completely different problem and not related to the one you posted in #2617. The icons in these new photos are fine, and now I look again in the post in apple life.ru #2629, and I see the icons there are fine too. In your post #2617 there is no black bar.

Yes, there are different problems but both XCODE8 vs XCODE5.

Zenith432 · November 20, 2017

@Slice: I can't reproduce this. When build with XCode8, I see the entire screen including F1: Help and version number. No black bar.

The compilations of egCreateFilledImage are both ok

XCODE8 uses sysv_abi for non-EFIAPI functions, but there's a prototype in libeg.h, so callers know about it. With no prototype - compilation error. With prototype, compiler know how to generate cross-abi calls.
XCODE5 uses ms_abi for all functions.

So these assembly functions don't help identify the problem. Since I can't reproduce, if you want, upload the XCODE8 binary for CloverX64.efi and I'll see if black bar happens and try debugging it.

Slice · November 20, 2017

Looking into loadpng.asm

00000000000027b8 4885C0                          test       rax, rax
00000000000027bb 7407                            je         0x27c4
                                       ; Basic Block Input Regs: rax r13 -  Killed Regs: r8
00000000000027bd 4D89E8                          mov        r8, r13
00000000000027c0 FFD0                            call       rax
00000000000027c2 EB05                            jmp        0x27c9
                                       ; Basic Block Input Regs: <nothing> -  Killed Regs: <nothing>
00000000000027c4 E800000000                      call       0x27c9                        ; XREF=0x27bb
                                       ; Basic Block Input Regs: rax rbx -  Killed Regs: rdx
00000000000027c9 85C0                            test       eax, eax                      ; XREF=0x27c2, 0x27c4

call rax???

@Slice: I can't reproduce this. When build with XCode8, I see the entire screen including F1: Help and version number. No black bar.

The compilations of egCreateFilledImage are both ok

XCODE8 uses sysv_abi for non-EFIAPI functions, but there's a prototype in libeg.h, so callers know about it. With no prototype - compilation error. With prototype, compiler know how to generate cross-abi calls.

XCODE5 uses ms_abi for all functions.

So these assembly functions don't help identify the problem. Since I can't reproduce, if you want, upload the XCODE8 binary for CloverX64.efi and I'll if black bar happens and try debugging it.

OK

CLOVER.efi.zip

Zenith432 · November 20, 2017

call rax is call to address is rax register holding a pointer to function.

@Slice

Attached is photo of screen with CLOVERX64.efi you uploaded in #2640.

No black bar.

Everything looks ok.

I double checked it's the one you uploaded, and deleted the previous build.

Maybe something in drivers64UEFI

Maybe something in config.plist

I don't know :surprised:

Slice · November 20, 2017

call rax is call to address is rax register holding a pointer to function.

@Slice

Attached is photo of screen with CLOVERX64.efi you uploaded in #2640.

No black bar.

Everything looks ok.

I double checked it's the one you uploaded, and deleted the previous build.

Maybe something in drivers64UEFI

Maybe something in config.plist

I don't know

May be CPU dependent.

I have no explanation. May be ScreenResolution = 1024x768?

One more result

#15648

Zenith432 · November 20, 2017

May be ScreenResolution = 1024x768?

This is your Clover at 1024x768

Slice · November 20, 2017

This is your Clover at 1024x768

And what about this one? http://www.insanelymac.com/forum/topic/284656-clover-general-discussion/page-783?p=2537527&do=findComment&comment=2537527

Slice · November 21, 2017

Several starts -> different pictures. It means non-initialized memory. But only if XCODE8?

Clover Problems and Solutions

Recommended Posts

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites