[RFC 0/5] EmbeddedPkg/PrePiLib: rework FfsProcessSection

[RFC 0/5] EmbeddedPkg/PrePiLib: rework FfsProcessSection

[Leif Lindholm]

https://bugzilla.tianocore.org/show_bug.cgi?id=2820 describes a build
failure caused by misanalysis by the compiler, but the problematic code
was pretty grotty, so here's an attempt at cleaning it up.

This set can also be accessed at:
https://github.com/leiflindholm/edk2/tree/embedded-fwvol-cleanup

Note: this code is only build tested.

Leif Lindholm (5):
  EmbeddedPkg/PrePiLib: style cleanup in FwVol.c
  EmbeddedPkg/PrePiLib: drop else if after return
  EmbeddedPkg/PrePiLib: refactor IS_SECTION2() handling
  EmbeddedPkg/PrePiLib: drop spurious re-init of CompressedData
  EmbeddedPkg/PrePiLib: break section extraction info into helper
    function

 EmbeddedPkg/Library/PrePiLib/FwVol.c | 214 ++++++++++++++++-----------
 1 file changed, 125 insertions(+), 89 deletions(-)

-- 
2.20.1

[RFC 1/5] EmbeddedPkg/PrePiLib: style cleanup in FwVol.c

[Leif Lindholm]

Move some curly brackets, change a couple of EFI_D_ to DEBUG_, and fix
some intentation.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 EmbeddedPkg/Library/PrePiLib/FwVol.c | 44 +++++++++++++---------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/EmbeddedPkg/Library/PrePiLib/FwVol.c b/EmbeddedPkg/Library/PrePiLib/FwVol.c
index 881506edddaf..90b5b4444002 100644
--- a/EmbeddedPkg/Library/PrePiLib/FwVol.c
+++ b/EmbeddedPkg/Library/PrePiLib/FwVol.c
@@ -361,7 +361,7 @@ FfsProcessSection (
         //
         // GetInfo failed
         //
-        DEBUG ((EFI_D_ERROR, "Decompress GetInfo Failed - %r\n", Status));
+        DEBUG ((DEBUG_ERROR, "Decompress GetInfo Failed - %r\n", Status));
         return EFI_NOT_FOUND;
       }
       //
@@ -392,38 +392,37 @@ FfsProcessSection (
       if (Section->Type == EFI_SECTION_COMPRESSION) {
         if (IS_SECTION2 (Section)) {
           CompressedData = (CHAR8 *) ((EFI_COMPRESSION_SECTION2 *) Section + 1);
-        }
-        else {
+        } else {
           CompressedData = (CHAR8 *) ((EFI_COMPRESSION_SECTION *) Section + 1);
         }
 
         Status = UefiDecompress (
-                    CompressedData,
-                    DstBuffer,
-                    ScratchBuffer
-                    );
+                   CompressedData,
+                   DstBuffer,
+                   ScratchBuffer
+                   );
       } else if (Section->Type == EFI_SECTION_GUID_DEFINED) {
         Status = ExtractGuidedSectionDecode (
-                    Section,
-                    &DstBuffer,
-                    ScratchBuffer,
-                    &AuthenticationStatus
-                    );
+                   Section,
+                   &DstBuffer,
+                   ScratchBuffer,
+                   &AuthenticationStatus
+                   );
       }
 
       if (EFI_ERROR (Status)) {
         //
         // Decompress failed
         //
-        DEBUG ((EFI_D_ERROR, "Decompress Failed - %r\n", Status));
+        DEBUG ((DEBUG_ERROR, "Decompress Failed - %r\n", Status));
         return EFI_NOT_FOUND;
       } else {
         return FfsProcessSection (
-                SectionType,
-                DstBuffer,
-                DstBufferSize,
-                OutputBuffer
-                );
+                 SectionType,
+                 DstBuffer,
+                 DstBufferSize,
+                 OutputBuffer
+                 );
        }
     }
 
@@ -756,17 +755,14 @@ FfsAnyFvFindFirstFile (
   Instance    = 0;
   *FileHandle = NULL;
 
-  while (1)
-  {
+  while (1) {
     Status = FfsFindNextVolume (Instance++, VolumeHandle);
-    if (EFI_ERROR (Status))
-    {
+    if (EFI_ERROR (Status)) {
       break;
     }
 
     Status = FfsFindNextFile (FileType, *VolumeHandle, FileHandle);
-    if (!EFI_ERROR (Status))
-    {
+    if (!EFI_ERROR (Status)) {
       break;
     }
   }
-- 
2.20.1

[RFC 2/5] EmbeddedPkg/PrePiLib: drop else if after return

[Leif Lindholm]

Simplify FfsProcessSection logic by breaking the continuation of the
main loop as a new if statement that executes if the very first test
doesn't end up returning.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 EmbeddedPkg/Library/PrePiLib/FwVol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/EmbeddedPkg/Library/PrePiLib/FwVol.c b/EmbeddedPkg/Library/PrePiLib/FwVol.c
index 90b5b4444002..fc40d8650be1 100644
--- a/EmbeddedPkg/Library/PrePiLib/FwVol.c
+++ b/EmbeddedPkg/Library/PrePiLib/FwVol.c
@@ -317,8 +317,9 @@ FfsProcessSection (
       }
 
       return EFI_SUCCESS;
-    } else if ((Section->Type == EFI_SECTION_COMPRESSION) || (Section->Type == EFI_SECTION_GUID_DEFINED)) {
+    }
 
+    if ((Section->Type == EFI_SECTION_COMPRESSION) || (Section->Type == EFI_SECTION_GUID_DEFINED)) {
       if (Section->Type == EFI_SECTION_COMPRESSION) {
         if (IS_SECTION2 (Section)) {
           CompressionSection2 = (EFI_COMPRESSION_SECTION2 *) Section;
-- 
2.20.1

[RFC 3/5] EmbeddedPkg/PrePiLib: refactor IS_SECTION2() handling

[Leif Lindholm]

There are a bunch of IS_SECTION2() conditional statements in
FfsProcessSection, really breaking up the readability.
Add a set of static helper functions instead.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 EmbeddedPkg/Library/PrePiLib/FwVol.c | 101 ++++++++++++++++-----------
 1 file changed, 61 insertions(+), 40 deletions(-)

diff --git a/EmbeddedPkg/Library/PrePiLib/FwVol.c b/EmbeddedPkg/Library/PrePiLib/FwVol.c
index fc40d8650be1..a0672c084471 100644
--- a/EmbeddedPkg/Library/PrePiLib/FwVol.c
+++ b/EmbeddedPkg/Library/PrePiLib/FwVol.c
@@ -266,6 +266,57 @@ FindFileEx (
   return EFI_NOT_FOUND;
 }
 
+STATIC
+UINTN
+FfsSectionHeaderSize (
+  IN EFI_COMMON_SECTION_HEADER                *Section
+  )
+{
+  if (IS_SECTION2 (Section)) {
+    return sizeof (EFI_COMMON_SECTION_HEADER2);
+  }
+
+  return sizeof (EFI_COMMON_SECTION_HEADER);
+}
+
+STATIC
+UINTN
+FfsSectionLength (
+  IN EFI_COMMON_SECTION_HEADER                *Section
+  )
+{
+  if (IS_SECTION2 (Section)) {
+    return SECTION2_SIZE (Section);
+  }
+
+  return SECTION_SIZE (Section);
+}
+
+STATIC
+UINTN
+FfsSectionCompressionType (
+  IN  EFI_COMMON_SECTION_HEADER               *Section
+  )
+{
+  if (IS_SECTION2 (Section)) {
+    return ((EFI_COMPRESSION_SECTION2 *)Section)->CompressionType;
+  }
+
+  return ((EFI_COMPRESSION_SECTION *)Section)->CompressionType;
+}
+
+STATIC
+UINTN
+FfsCompressionSectionHeaderSize (
+  IN  EFI_COMMON_SECTION_HEADER               *Section
+  )
+{
+  if (IS_SECTION2 (Section)) {
+    return sizeof (EFI_COMPRESSION_SECTION2);
+  }
+
+  return sizeof (EFI_COMPRESSION_SECTION);
+}
 
 /**
   Go through the file to search SectionType section,
@@ -289,8 +340,6 @@ FfsProcessSection (
   EFI_STATUS                              Status;
   UINT32                                  SectionLength;
   UINT32                                  ParsedLength;
-  EFI_COMPRESSION_SECTION                 *CompressionSection;
-  EFI_COMPRESSION_SECTION2                *CompressionSection2;
   UINT32                                  DstBufferSize;
   VOID                                    *ScratchBuffer;
   UINT32                                  ScratchBufferSize;
@@ -310,39 +359,22 @@ FfsProcessSection (
     }
 
     if (Section->Type == SectionType) {
-      if (IS_SECTION2 (Section)) {
-        *OutputBuffer = (VOID *)((UINT8 *) Section + sizeof (EFI_COMMON_SECTION_HEADER2));
-      } else {
-        *OutputBuffer = (VOID *)((UINT8 *) Section + sizeof (EFI_COMMON_SECTION_HEADER));
-      }
+      *OutputBuffer = (VOID *)((UINT8 *)Section + FfsSectionHeaderSize (Section));
 
       return EFI_SUCCESS;
     }
 
     if ((Section->Type == EFI_SECTION_COMPRESSION) || (Section->Type == EFI_SECTION_GUID_DEFINED)) {
       if (Section->Type == EFI_SECTION_COMPRESSION) {
-        if (IS_SECTION2 (Section)) {
-          CompressionSection2 = (EFI_COMPRESSION_SECTION2 *) Section;
-          SectionLength       = SECTION2_SIZE (Section);
+        SectionLength = FfsSectionLength (Section);
 
-          if (CompressionSection2->CompressionType != EFI_STANDARD_COMPRESSION) {
-            return EFI_UNSUPPORTED;
-          }
-
-          CompressedData = (CHAR8 *) ((EFI_COMPRESSION_SECTION2 *) Section + 1);
-          CompressedDataLength = (UINT32) SectionLength - sizeof (EFI_COMPRESSION_SECTION2);
-        } else {
-          CompressionSection  = (EFI_COMPRESSION_SECTION *) Section;
-          SectionLength       = SECTION_SIZE (Section);
-
-          if (CompressionSection->CompressionType != EFI_STANDARD_COMPRESSION) {
-            return EFI_UNSUPPORTED;
-          }
-
-          CompressedData = (CHAR8 *) ((EFI_COMPRESSION_SECTION *) Section + 1);
-          CompressedDataLength = (UINT32) SectionLength - sizeof (EFI_COMPRESSION_SECTION);
+        if (FfsSectionCompressionType (Section) != EFI_STANDARD_COMPRESSION) {
+          return EFI_UNSUPPORTED;
         }
 
+        CompressedData = (VOID *)((UINTN)Section + FfsCompressionSectionHeaderSize (Section));
+        CompressedDataLength = SectionLength - FfsCompressionSectionHeaderSize (Section);
+
         Status = UefiDecompressGetInfo (
                    CompressedData,
                    CompressedDataLength,
@@ -383,19 +415,12 @@ FfsProcessSection (
       // DstBuffer still is one section. Adjust DstBuffer offset, skip EFI section header
       // to make section data at page alignment.
       //
-      if (IS_SECTION2 (Section))
-        DstBuffer = (UINT8 *)DstBuffer + EFI_PAGE_SIZE - sizeof (EFI_COMMON_SECTION_HEADER2);
-      else
-        DstBuffer = (UINT8 *)DstBuffer + EFI_PAGE_SIZE - sizeof (EFI_COMMON_SECTION_HEADER);
+      DstBuffer = (UINT8 *)DstBuffer + EFI_PAGE_SIZE - FfsSectionHeaderSize (Section);
       //
       // Call decompress function
       //
       if (Section->Type == EFI_SECTION_COMPRESSION) {
-        if (IS_SECTION2 (Section)) {
-          CompressedData = (CHAR8 *) ((EFI_COMPRESSION_SECTION2 *) Section + 1);
-        } else {
-          CompressedData = (CHAR8 *) ((EFI_COMPRESSION_SECTION *) Section + 1);
-        }
+        CompressedData = (VOID *)((UINTN)Section + FfsCompressionSectionHeaderSize (Section));
 
         Status = UefiDecompress (
                    CompressedData,
@@ -427,11 +452,7 @@ FfsProcessSection (
        }
     }
 
-    if (IS_SECTION2 (Section)) {
-      SectionLength = SECTION2_SIZE (Section);
-    } else {
-      SectionLength = SECTION_SIZE (Section);
-    }
+    SectionLength = FfsSectionLength (Section);
     //
     // SectionLength is adjusted it is 4 byte aligned.
     // Go to the next section
-- 
2.20.1

[RFC 4/5] EmbeddedPkg/PrePiLib: drop spurious re-init of CompressedData

[Leif Lindholm]

After the refactoring, it is very clear that CompressedData is
initialized twice, using exactly the same values. Drop the
second one.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 EmbeddedPkg/Library/PrePiLib/FwVol.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/EmbeddedPkg/Library/PrePiLib/FwVol.c b/EmbeddedPkg/Library/PrePiLib/FwVol.c
index a0672c084471..083bc27efead 100644
--- a/EmbeddedPkg/Library/PrePiLib/FwVol.c
+++ b/EmbeddedPkg/Library/PrePiLib/FwVol.c
@@ -420,8 +420,6 @@ FfsProcessSection (
       // Call decompress function
       //
       if (Section->Type == EFI_SECTION_COMPRESSION) {
-        CompressedData = (VOID *)((UINTN)Section + FfsCompressionSectionHeaderSize (Section));
-
         Status = UefiDecompress (
                    CompressedData,
                    DstBuffer,
-- 
2.20.1

[RFC 5/5] EmbeddedPkg/PrePiLib: break section extraction info into helper function

[Leif Lindholm]

Create a new helper function FfsGetExtractionInfo, which handles figuring
out the buffer sizes needed for extracting UefiCompressed or
GuidedSection sections, and also hides away some of the differences
between the two, getting rid of a bunch of local variables.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 EmbeddedPkg/Library/PrePiLib/FwVol.c | 90 +++++++++++++++++-----------
 1 file changed, 55 insertions(+), 35 deletions(-)

diff --git a/EmbeddedPkg/Library/PrePiLib/FwVol.c b/EmbeddedPkg/Library/PrePiLib/FwVol.c
index 083bc27efead..d0f91efa77a1 100644
--- a/EmbeddedPkg/Library/PrePiLib/FwVol.c
+++ b/EmbeddedPkg/Library/PrePiLib/FwVol.c
@@ -318,6 +318,50 @@ FfsCompressionSectionHeaderSize (
   return sizeof (EFI_COMPRESSION_SECTION);
 }
 
+STATIC
+EFI_STATUS
+FfsGetExtractionInfo (
+  IN      EFI_COMMON_SECTION_HEADER               *Section,
+  OUT     UINT32                                  *SectionLength,
+  IN OUT  VOID                                    **SrcBuffer,
+  OUT     UINT32                                  *DstBufferSize,
+  OUT     UINT32                                  *ScratchBufferSize
+  )
+{
+  EFI_STATUS  Status;
+
+  if (Section->Type == EFI_SECTION_COMPRESSION) {
+    *SectionLength = FfsSectionLength (Section);
+
+    if (FfsSectionCompressionType (Section) != EFI_STANDARD_COMPRESSION) {
+      return EFI_UNSUPPORTED;
+    }
+
+    *SrcBuffer = (VOID *)((UINTN)Section + FfsCompressionSectionHeaderSize (Section));
+    Status = UefiDecompressGetInfo (
+               *SrcBuffer,
+               *SectionLength - FfsCompressionSectionHeaderSize (Section),
+               DstBufferSize,
+               ScratchBufferSize
+               );
+  } else if (Section->Type == EFI_SECTION_GUID_DEFINED) {
+    UINT16 Ignored;
+
+    *SrcBuffer = Section;
+
+    Status = ExtractGuidedSectionGetInfo (
+               *SrcBuffer,
+               DstBufferSize,
+               ScratchBufferSize,
+               &Ignored      // SectionAttribute not used by this library
+               );
+  } else {
+    Status = EFI_UNSUPPORTED;
+  }
+
+  return Status;
+}
+
 /**
   Go through the file to search SectionType section,
   when meeting an encapsuled section.
@@ -344,15 +388,11 @@ FfsProcessSection (
   VOID                                    *ScratchBuffer;
   UINT32                                  ScratchBufferSize;
   VOID                                    *DstBuffer;
-  UINT16                                  SectionAttribute;
-  UINT32                                  AuthenticationStatus;
-  CHAR8                                   *CompressedData;
-  UINTN                                   CompressedDataLength;
-
+  VOID                                    *SrcBuffer;
 
   *OutputBuffer = NULL;
   ParsedLength  = 0;
-  Status        = EFI_NOT_FOUND;
+
   while (ParsedLength < SectionSize) {
     if (IS_SECTION2 (Section)) {
       ASSERT (SECTION2_SIZE (Section) > 0x00FFFFFF);
@@ -364,32 +404,11 @@ FfsProcessSection (
       return EFI_SUCCESS;
     }
 
+    SectionLength = FfsSectionLength (Section);
+
     if ((Section->Type == EFI_SECTION_COMPRESSION) || (Section->Type == EFI_SECTION_GUID_DEFINED)) {
-      if (Section->Type == EFI_SECTION_COMPRESSION) {
-        SectionLength = FfsSectionLength (Section);
-
-        if (FfsSectionCompressionType (Section) != EFI_STANDARD_COMPRESSION) {
-          return EFI_UNSUPPORTED;
-        }
-
-        CompressedData = (VOID *)((UINTN)Section + FfsCompressionSectionHeaderSize (Section));
-        CompressedDataLength = SectionLength - FfsCompressionSectionHeaderSize (Section);
-
-        Status = UefiDecompressGetInfo (
-                   CompressedData,
-                   CompressedDataLength,
-                   &DstBufferSize,
-                   &ScratchBufferSize
-                   );
-      } else if (Section->Type == EFI_SECTION_GUID_DEFINED) {
-        Status = ExtractGuidedSectionGetInfo (
-                   Section,
-                   &DstBufferSize,
-                   &ScratchBufferSize,
-                   &SectionAttribute
-                   );
-      }
-
+      Status = FfsGetExtractionInfo (Section, &SectionLength, &SrcBuffer,
+                                     &DstBufferSize, &ScratchBufferSize);
       if (EFI_ERROR (Status)) {
         //
         // GetInfo failed
@@ -421,16 +440,18 @@ FfsProcessSection (
       //
       if (Section->Type == EFI_SECTION_COMPRESSION) {
         Status = UefiDecompress (
-                   CompressedData,
+                   SrcBuffer,
                    DstBuffer,
                    ScratchBuffer
                    );
       } else if (Section->Type == EFI_SECTION_GUID_DEFINED) {
+        UINT32  Ignored;
+
         Status = ExtractGuidedSectionDecode (
-                   Section,
+                   SrcBuffer,
                    &DstBuffer,
                    ScratchBuffer,
-                   &AuthenticationStatus
+                   &Ignored  // AuthenticationStatus not used by this library
                    );
       }
 
@@ -450,7 +471,6 @@ FfsProcessSection (
        }
     }
 
-    SectionLength = FfsSectionLength (Section);
     //
     // SectionLength is adjusted it is 4 byte aligned.
     // Go to the next section
-- 
2.20.1

Re: [RFC 0/5] EmbeddedPkg/PrePiLib: rework FfsProcessSection

[Ard Biesheuvel]

On 7/1/20 10:01 PM, Leif Lindholm wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2820 describes a build
> failure caused by misanalysis by the compiler, but the problematic code
> was pretty grotty, so here's an attempt at cleaning it up.
> 
> This set can also be accessed at:
> https://github.com/leiflindholm/edk2/tree/embedded-fwvol-cleanup
> 
> Note: this code is only build tested.
> 
> Leif Lindholm (5):
>    EmbeddedPkg/PrePiLib: style cleanup in FwVol.c
>    EmbeddedPkg/PrePiLib: drop else if after return
>    EmbeddedPkg/PrePiLib: refactor IS_SECTION2() handling
>    EmbeddedPkg/PrePiLib: drop spurious re-init of CompressedData
>    EmbeddedPkg/PrePiLib: break section extraction info into helper
>      function
> 

For the series:

Tested-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>


>   EmbeddedPkg/Library/PrePiLib/FwVol.c | 214 ++++++++++++++++-----------
>   1 file changed, 125 insertions(+), 89 deletions(-)
>