[PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: udf_fixes_cleanups

Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for
IA32 and X64 with clang-3.8, after the UDF introduction.

Patches #1 and #5 are cleanups that I felt fit before patch #2 and after
patch #4, respectively.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>

Thanks
Laszlo

Laszlo Ersek (5):
  MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA
    req
  MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req
    succeeds
  MdeModulePkg/UdfDxe: replace zero-init of local variables with
    ZeroMem()
  MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators
  MdeModulePkg/PartitionDxe: remove always false comparison

 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c            | 9 +++++++--
 MdeModulePkg/Universal/Disk/UdfDxe/File.c                 | 6 ++++--
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++
 3 files changed, 16 insertions(+), 4 deletions(-)

-- 
2.14.1.3.gb7cf6e02401b

[PATCH 1/5] MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA req

[Laszlo Ersek]

In the ReadFile() function, if "RecordingFlags" is INLINE_DATA, then we
cover the following values of "ReadFileInfo->Flags":

- READ_FILE_GET_FILESIZE
- READ_FILE_ALLOCATE_AND_READ
- READ_FILE_SEEK_AND_READ

We don't do anything (just proceed to the end of the function) if
"ReadFileInfo->Flags" is anything else.

In reality the above three values cover the domain of the
UDF_READ_FILE_FLAGS enum type fully, and "ReadFileInfo->Flags" is only
ever set internally to UdfDxe. Therefore any other flag value would be a
bug in UdfDxe.

ASSERT() specifically that "ReadFileInfo->Flags" has been set correctly,
so that the reader is not left wondering what happens if none of the enum
constants match.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 7d7f722188c5..a2ca65e5dfe8 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -953,10 +953,13 @@ ReadFile (
         (VOID *)((UINT8 *)Data + ReadFileInfo->FilePosition),
         ReadFileInfo->FileDataSize
         );
 
       ReadFileInfo->FilePosition += ReadFileInfo->FileDataSize;
+    } else {
+      ASSERT (FALSE);
+      return EFI_INVALID_PARAMETER;
     }
 
     break;
   case LONG_ADS_SEQUENCE:
   case SHORT_ADS_SEQUENCE:
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 2/5] MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req succeeds

[Laszlo Ersek]

Ard reports that clang-3.8 correctly flags the following issue in the
ReadFile() function:

If "RecordingFlags" is INLINE_DATA, then there are three paths through the
code where we mean to return success, but forget to set Status
accordingly:

(1) when "ReadFileInfo->Flags" is READ_FILE_GET_FILESIZE, or

(2) when "ReadFileInfo->Flags" is READ_FILE_ALLOCATE_AND_READ and
    AllocatePool() succeeds, or

(3) when "ReadFileInfo->Flags" is READ_FILE_SEEK_AND_READ.

Set "Status" to EFI_SUCCESS when we are done processing the INLINE_DATA
request, i.e., when we reach the corresponding "break" statament under the
INLINE_DATA case label.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index a2ca65e5dfe8..0de9c71c250f 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -958,11 +958,13 @@ ReadFile (
     } else {
       ASSERT (FALSE);
       return EFI_INVALID_PARAMETER;
     }
 
+    Status = EFI_SUCCESS;
     break;
+
   case LONG_ADS_SEQUENCE:
   case SHORT_ADS_SEQUENCE:
     //
     // This FE/EFE contains a run of Allocation Descriptors. Get data + size
     // for start reading them out.
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem()

[Laszlo Ersek]

In edk2, initialization of local variables is forbidden, both for
stylistic reasons and because such initialization may generate calls to
compiler intrinsics.

For the following initialization in UdfRead():

  CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };

clang-3.8 generates a memset() call, when building UdfDxe for IA32, which
then fails to link.

Replace the initialization with ZeroMem().

Do the same to "FilePath" in UdfOpen().

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 8b9339567f8e..e7159ff861f7 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -174,15 +174,16 @@ UdfOpen (
 {
   EFI_TPL                     OldTpl;
   EFI_STATUS                  Status;
   PRIVATE_UDF_FILE_DATA       *PrivFileData;
   PRIVATE_UDF_SIMPLE_FS_DATA  *PrivFsData;
-  CHAR16                      FilePath[UDF_PATH_LENGTH] = { 0 };
+  CHAR16                      FilePath[UDF_PATH_LENGTH];
   UDF_FILE_INFO               File;
   PRIVATE_UDF_FILE_DATA       *NewPrivFileData;
   CHAR16                      *TempFileName;
 
+  ZeroMem (FilePath, sizeof FilePath);
   OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
 
   if (This == NULL || NewHandle == NULL || FileName == NULL) {
     Status = EFI_INVALID_PARAMETER;
     goto Error_Invalid_Params;
@@ -322,14 +323,15 @@ UdfRead (
   EFI_BLOCK_IO_PROTOCOL           *BlockIo;
   EFI_DISK_IO_PROTOCOL            *DiskIo;
   UDF_FILE_INFO                   FoundFile;
   UDF_FILE_IDENTIFIER_DESCRIPTOR  *NewFileIdentifierDesc;
   VOID                            *NewFileEntryData;
-  CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
+  CHAR16                          FileName[UDF_FILENAME_LENGTH];
   UINT64                          FileSize;
   UINT64                          BufferSizeUint64;
 
+  ZeroMem (FileName, sizeof FileName);
   OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
 
   if (This == NULL || BufferSize == NULL || (*BufferSize != 0 &&
                                              Buffer == NULL)) {
     Status = EFI_INVALID_PARAMETER;
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators

[Laszlo Ersek]

In edk2, the division and shifting of 64-bit values are forbidden with
C-language operators, because the compiler may generate intrinsic calls
for them.

For example, clang-3.8 emits a call to "__umoddi3" for

  UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize

in PartitionInstallUdfChildHandles(), if PartitionDxe is built for IA32,
which then fails to link.

UDF_LOGICAL_SECTOR_SIZE has type UINT64, while
EFI_BLOCK_IO_MEDIA.BlockSize has type UINT32(). Replace the % operator
with a DivU64x32Remainder() call.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
index c1d44809bfd2..c491ef25f47e 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
@@ -234,10 +234,11 @@ PartitionInstallUdfChildHandles (
   IN  EFI_BLOCK_IO_PROTOCOL        *BlockIo,
   IN  EFI_BLOCK_IO2_PROTOCOL       *BlockIo2,
   IN  EFI_DEVICE_PATH_PROTOCOL     *DevicePath
   )
 {
+  UINT32                       RemainderByMediaBlockSize;
   EFI_STATUS                   Status;
   EFI_BLOCK_IO_MEDIA           *Media;
   EFI_DEVICE_PATH_PROTOCOL     *DevicePathNode;
   EFI_GUID                     *VendorDefinedGuid;
   EFI_GUID                     UdfDevPathGuid = EFI_UDF_DEVICE_PATH_GUID;
@@ -246,11 +247,16 @@ PartitionInstallUdfChildHandles (
   Media = BlockIo->Media;
 
   //
   // Check if UDF logical block size is multiple of underlying device block size
   //
-  if ((UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize) != 0 ||
+  DivU64x32Remainder (
+    UDF_LOGICAL_SECTOR_SIZE,   // Dividend
+    Media->BlockSize,          // Divisor
+    &RemainderByMediaBlockSize // Remainder
+    );
+  if (RemainderByMediaBlockSize != 0 ||
       Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE) {
     return EFI_NOT_FOUND;
   }
 
   DevicePathNode = DevicePath;
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 5/5] MdeModulePkg/PartitionDxe: remove always false comparison

[Laszlo Ersek]

In the expression

  (RemainderByMediaBlockSize != 0 ||
   Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE)

the second expression is only evaluated if the first expression is false.

If the first expression is false, i.e.,

  RemainderByMediaBlockSize == 0

then UDF_LOGICAL_SECTOR_SIZE is a whole multiple of "Media->BlockSize",
which implies

  UDF_LOGICAL_SECTOR_SIZE >= Media->BlockSize.

Therefore whenever

  Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE

is evaluated, it is false.

The expression

  ((expression) || FALSE)

is equivalent to

  (expression).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
index c491ef25f47e..3347b48421a8 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
@@ -252,12 +252,11 @@ PartitionInstallUdfChildHandles (
   DivU64x32Remainder (
     UDF_LOGICAL_SECTOR_SIZE,   // Dividend
     Media->BlockSize,          // Divisor
     &RemainderByMediaBlockSize // Remainder
     );
-  if (RemainderByMediaBlockSize != 0 ||
-      Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE) {
+  if (RemainderByMediaBlockSize != 0) {
     return EFI_NOT_FOUND;
   }
 
   DevicePathNode = DevicePath;
   while (!IsDevicePathEnd (DevicePathNode)) {
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Shi, Steven]

Hi Laszlo,
How could we configure the Qemu and test the UDF driver on OVMF?  
BTW, how could we configure the Qemu to create a full feature scope machine to include all possible devices in it, e.g. USB, ISA, SD/MMC, network etc. 


Steven Shi
Intel\SSG\STO\UEFI Firmware

Tel: +86 021-61166522
iNet: 821-6522

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Sunday, September 10, 2017 8:13 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: udf_fixes_cleanups
> 
> Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for
> IA32 and X64 with clang-3.8, after the UDF introduction.
> 
> Patches #1 and #5 are cleanups that I felt fit before patch #2 and after
> patch #4, respectively.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Paulo Alcantara <pcacjr@zytor.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (5):
>   MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA
>     req
>   MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req
>     succeeds
>   MdeModulePkg/UdfDxe: replace zero-init of local variables with
>     ZeroMem()
>   MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators
>   MdeModulePkg/PartitionDxe: remove always false comparison
> 
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c            | 9 +++++++--
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c                 | 6 ++++--
>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> --
> 2.14.1.3.gb7cf6e02401b
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Laszlo Ersek]

On 09/10/17 06:24, Shi, Steven wrote:
> Hi Laszlo,
> How could we configure the Qemu and test the UDF driver on OVMF?  

I guess you would format e.g. a DVD image with UDF, and attach it to
QEMU like any other CD-ROM.

> BTW, how could we configure the Qemu to create a full feature scope machine to include all possible devices in it, e.g. USB, ISA, SD/MMC, network etc. 

This question is impossible to answer, there are so many device models
in QEMU. Instead, if you have a specific driver in edk2 that you would
like to test, I'd recommend adding a device model to the machine
configuration just for that. If you have several drivers in mind, repeat
until happy.

Either way, I'd certainly not recommend the raw QEMU command line for
this; I recommend libvirtd, and virt-manager + "virsh edit".

http://libvirt.org/formatdomain.html

Thanks
Laszlo


> 
> 
> Steven Shi
> Intel\SSG\STO\UEFI Firmware
> 
> Tel: +86 021-61166522
> iNet: 821-6522
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Sunday, September 10, 2017 8:13 AM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng,
>> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
>>
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: udf_fixes_cleanups
>>
>> Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for
>> IA32 and X64 with clang-3.8, after the UDF introduction.
>>
>> Patches #1 and #5 are cleanups that I felt fit before patch #2 and after
>> patch #4, respectively.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Paulo Alcantara <pcacjr@zytor.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (5):
>>   MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA
>>     req
>>   MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req
>>     succeeds
>>   MdeModulePkg/UdfDxe: replace zero-init of local variables with
>>     ZeroMem()
>>   MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators
>>   MdeModulePkg/PartitionDxe: remove always false comparison
>>
>>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c            | 9 +++++++--
>>  MdeModulePkg/Universal/Disk/UdfDxe/File.c                 | 6 ++++--
>>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++
>>  3 files changed, 16 insertions(+), 4 deletions(-)
>>
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Laszlo Ersek]

Hi Steven,

On 09/10/17 10:38, Laszlo Ersek wrote:
> On 09/10/17 06:24, Shi, Steven wrote:
>> Hi Laszlo,
>> How could we configure the Qemu and test the UDF driver on OVMF?  
> 
> I guess you would format e.g. a DVD image with UDF, and attach it to
> QEMU like any other CD-ROM.

I tried to look into this -- I tried several things, but nothing
produced an UDF image file that, when attached to the VM, would show up
in the UEFI shell as FSn:

Google returned a bunch of pages, but all I found was:
- tips that didn't work (see above),
- confused users (like me) looking for solutions.

So, at the moment, I have no idea how authoring UDF DVD images is
possible on Linux, so that they'd be recognized in edk2.

(I'm interested in the edk2 UDF driver not because I want to "author"
UDF DVD images (ISO9660+ElTorito works just fine), but because some
optical media images that were given to me are formatted UDF-only. They
can be translated into ISO9660+ElTorito off-line, but that's a chore.)

Thanks,
Laszlo

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Shi, Steven]

OK. Does the UDF image you created correctly show up as CD-ROM content in Linux, e.g Fedora?


Steven Shi
Intel\SSG\STO\UEFI Firmware

Tel: +86 021-61166522
iNet: 821-6522

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Sunday, September 10, 2017 9:52 PM
> To: Shi, Steven <steven.shi@intel.com>; edk2-devel-01 <edk2-
> devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
> 
> Hi Steven,
> 
> On 09/10/17 10:38, Laszlo Ersek wrote:
> > On 09/10/17 06:24, Shi, Steven wrote:
> >> Hi Laszlo,
> >> How could we configure the Qemu and test the UDF driver on OVMF?
> >
> > I guess you would format e.g. a DVD image with UDF, and attach it to
> > QEMU like any other CD-ROM.
> 
> I tried to look into this -- I tried several things, but nothing
> produced an UDF image file that, when attached to the VM, would show up
> in the UEFI shell as FSn:
> 
> Google returned a bunch of pages, but all I found was:
> - tips that didn't work (see above),
> - confused users (like me) looking for solutions.
> 
> So, at the moment, I have no idea how authoring UDF DVD images is
> possible on Linux, so that they'd be recognized in edk2.
> 
> (I'm interested in the edk2 UDF driver not because I want to "author"
> UDF DVD images (ISO9660+ElTorito works just fine), but because some
> optical media images that were given to me are formatted UDF-only. They
> can be translated into ISO9660+ElTorito off-line, but that's a chore.)
> 
> Thanks,
> Laszlo

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Paulo Alcantara]

On 09/09/2017 21:12, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: udf_fixes_cleanups
> 
> Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for
> IA32 and X64 with clang-3.8, after the UDF introduction.
> 
> Patches #1 and #5 are cleanups that I felt fit before patch #2 and after
> patch #4, respectively.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Paulo Alcantara <pcacjr@zytor.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (5):
>    MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA
>      req
>    MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req
>      succeeds
>    MdeModulePkg/UdfDxe: replace zero-init of local variables with
>      ZeroMem()
>    MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators
>    MdeModulePkg/PartitionDxe: remove always false comparison
> 
>   MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c            | 9 +++++++--
>   MdeModulePkg/Universal/Disk/UdfDxe/File.c                 | 6 ++++--
>   MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++
>   3 files changed, 16 insertions(+), 4 deletions(-)
> 

Looks good to me. Tested it with OVMF X64 and AARCH64.

Reviewed-by: Paulo Alcantara <pcacjr@zytor.com>

Thanks!
Paulo

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Paulo Alcantara]

On 10/09/2017 11:27, Shi, Steven wrote:
> OK. Does the UDF image you created correctly show up as CD-ROM content in Linux, e.g Fedora?

The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso) does 
not contain a valid UDF file system, so you cannot use it for testing.

To make sure it really doesn't, I used a "Philips UDF Conformance Tool" 
that you can grab in [1], and the tool didn't find any valid UDF file 
system in it.

I've been using an unmodified Windows 10 Enterprise image that does 
contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to perform 
my tests.

Additionally, I use an USB stick that I format it with 'sudo mkudffs -b 
512 --media-type=hd /dev/sdX' and copy some files to it for testing

BTW, when testing with OVMF AARCH64, I was using my USB stick that 
showed up correctly as 'fsX' in UEFI shell, but with the Windows 10 
Enterprise ISO image, it didn't. I looked at the logs to see what was 
going on and I found out that the emulated CD-ROM drive is reporting a 
block size of 512 instead of 2048 -- which seems wrong to me. With 
'qemu-system-x86_64 -cdrom', it reports a block size of 2048.

The Partition driver is still able to find a valid ElTorito partition 
because, regardless the device block size, it always use a logical block 
size of 2048 starting at 32K. In UDF, when searching for AVDPs (Anchor 
Volume Descriptor Pointers), we search for them at fixed locations: 256, 
N - 256, N and 512 -- but, with a block size of 512, the locations 
change to 1024, N - 1024, N and 2048 -- thus breaking the volume 
recognition sequence.

For testing the Windows image with a block size of 512, I used the 
comformance tool with './udf_test -verbose 60 -blocksize 512 
~/img/win_ent10.iso' and it failed to find a valid UDF file system. But 
with a block size of 2048, it worked.

Is there any reason for reporting a block size of 512 when using 
'-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing 
something here?

Thanks!
Paulo

[1] - https://www.lscdweb.com/registered/udf_verifier.html

> 
> 
> Steven Shi
> Intel\SSG\STO\UEFI Firmware
> 
> Tel: +86 021-61166522
> iNet: 821-6522
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Sunday, September 10, 2017 9:52 PM
>> To: Shi, Steven <steven.shi@intel.com>; edk2-devel-01 <edk2-
>> devel@lists.01.org>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng,
>> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
>>
>> Hi Steven,
>>
>> On 09/10/17 10:38, Laszlo Ersek wrote:
>>> On 09/10/17 06:24, Shi, Steven wrote:
>>>> Hi Laszlo,
>>>> How could we configure the Qemu and test the UDF driver on OVMF?
>>>
>>> I guess you would format e.g. a DVD image with UDF, and attach it to
>>> QEMU like any other CD-ROM.
>>
>> I tried to look into this -- I tried several things, but nothing
>> produced an UDF image file that, when attached to the VM, would show up
>> in the UEFI shell as FSn:
>>
>> Google returned a bunch of pages, but all I found was:
>> - tips that didn't work (see above),
>> - confused users (like me) looking for solutions.
>>
>> So, at the moment, I have no idea how authoring UDF DVD images is
>> possible on Linux, so that they'd be recognized in edk2.
>>
>> (I'm interested in the edk2 UDF driver not because I want to "author"
>> UDF DVD images (ISO9660+ElTorito works just fine), but because some
>> optical media images that were given to me are formatted UDF-only. They
>> can be translated into ISO9660+ElTorito off-line, but that's a chore.)
>>
>> Thanks,
>> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Laszlo Ersek]

On 09/10/17 17:51, Paulo Alcantara wrote:
> 
> 
> On 10/09/2017 11:27, Shi, Steven wrote:
>> OK. Does the UDF image you created correctly show up as CD-ROM content
>> in Linux, e.g Fedora?
> 
> The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso) does
> not contain a valid UDF file system, so you cannot use it for testing.
> 
> To make sure it really doesn't, I used a "Philips UDF Conformance Tool"
> that you can grab in [1], and the tool didn't find any valid UDF file
> system in it.
> 
> I've been using an unmodified Windows 10 Enterprise image that does
> contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to perform
> my tests.
> 
> Additionally, I use an USB stick that I format it with 'sudo mkudffs -b
> 512 --media-type=hd /dev/sdX' and copy some files to it for testing
> 
> BTW, when testing with OVMF AARCH64, I was using my USB stick that
> showed up correctly as 'fsX' in UEFI shell, but with the Windows 10
> Enterprise ISO image, it didn't. I looked at the logs to see what was
> going on and I found out that the emulated CD-ROM drive is reporting a
> block size of 512 instead of 2048 -- which seems wrong to me. With
> 'qemu-system-x86_64 -cdrom', it reports a block size of 2048.
> 
> The Partition driver is still able to find a valid ElTorito partition
> because, regardless the device block size, it always use a logical block
> size of 2048 starting at 32K. In UDF, when searching for AVDPs (Anchor
> Volume Descriptor Pointers), we search for them at fixed locations: 256,
> N - 256, N and 512 -- but, with a block size of 512, the locations
> change to 1024, N - 1024, N and 2048 -- thus breaking the volume
> recognition sequence.
> 
> For testing the Windows image with a block size of 512, I used the
> comformance tool with './udf_test -verbose 60 -blocksize 512
> ~/img/win_ent10.iso' and it failed to find a valid UDF file system. But
> with a block size of 2048, it worked.
> 
> Is there any reason for reporting a block size of 512 when using
> '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing
> something here?

"-cdrom" is a legacy QEMU option, a shorthand that expands to a -drive
option, and at least one -device option (it could expand to multiple
-device options). And, this expansion can be different on different
machine types. On i440fx machine types, -cdrom can mean an IDE CD, on
q35, an AHCI CD, an aarch64/virt, a virtio-scsi-pci controller and a
scsi-cd. For this reason, it is always best to use complete -drive and
-device specifications (which is equivalent to saying it is best to
always use libvirt).

Now, the above does not imlpy that no bug can exist in this space. Can
you run the "info qtree" monitor command, in both cases, and compare the
output with regard to the CD-ROM?

Thanks
Laszlo

> Thanks!
> Paulo
> 
> [1] - https://www.lscdweb.com/registered/udf_verifier.html

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Ard Biesheuvel]

On 10 September 2017 at 01:12, Laszlo Ersek <lersek@redhat.com> wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: udf_fixes_cleanups
>
> Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for
> IA32 and X64 with clang-3.8, after the UDF introduction.
>
> Patches #1 and #5 are cleanups that I felt fit before patch #2 and after
> patch #4, respectively.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Paulo Alcantara <pcacjr@zytor.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
>

Series Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Shi, Steven]

Hi Laszlo,
Your code is good. But there is a "Null Pointer Use" issue, which is a undefined behavior in C spec, in edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:696, column: 7 and line:707, column: 14.

Line695:
  FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
  if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
   ...

The above FileIdentifierDesc can be NULL if you run a .efi in a Udf partition. And the later IS_FID_DIRECTORY_FILE (FileIdentifierDesc) is unsafe if FileIdentifierDesc == NULL. You can test it with below extra code which add debug output when FileIdentifierDesc == NULL, and check the log after load and run something in a Udf partition.

+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -693,6 +693,11 @@ UdfSetPosition (
   PrivFileData = PRIVATE_UDF_FILE_DATA_FROM_THIS (This);

   FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
+  if (FileIdentifierDesc == NULL) {
+    DEBUG ((EFI_D_ERROR, "FileIdentifierDesc == NULL!\n"));
+    return Status;
+  }
+
   if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
     //
     // If the file handle is a directory, the _only_ position that may be set is

I followed the Paulo's suggestion, and found this issue when using the Udf partition of Windows 10 Enterprise ISO image on qemu. Below is my qemu command:
/usr/local/bin/qemu-system-x86_64 -m 5120 -enable-kvm  -machine pc-q35-2.9 -bios OVMF.fd -serial file:serial.log  -cdrom /media/jshi19/My\ Passport/OSImage/ISO/winserver/en_windows_server_2016_x64_dvd_9327751.iso

I've submitted a bug for this issue: https://bugzilla.tianocore.org/show_bug.cgi?id=704. 

I found this issue by edk2 llvm undefined behavior sanitizer, and below is the sanitizer output. FYI.
/home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02B8, column:0x0007 ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called: 
UdfSetPosition
/home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c:696:7
EfiShellFindFilesInDir
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2136:18
ShellSearchHandle
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2413:14
EfiShellFindFiles
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2586:18
EfiShellOpenFileList
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2666:12
ShellOpenFileMetaArg
/home/jshi19/wksp_efi/edk2/ShellPkg/Library/UefiShellLib/UefiShellLib.c:1570:14

/home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02C3, column:0x000E ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called:
...

Steven Shi
Intel\SSG\STO\UEFI Firmware

Tel: +86 021-61166522
iNet: 821-6522

> -----Original Message-----
> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> Sent: Sunday, September 10, 2017 11:52 PM
> To: Shi, Steven <steven.shi@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
> 
> 
> 
> On 10/09/2017 11:27, Shi, Steven wrote:
> > OK. Does the UDF image you created correctly show up as CD-ROM
> content in Linux, e.g Fedora?
> 
> The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso) does
> not contain a valid UDF file system, so you cannot use it for testing.
> 
> To make sure it really doesn't, I used a "Philips UDF Conformance Tool"
> that you can grab in [1], and the tool didn't find any valid UDF file
> system in it.
> 
> I've been using an unmodified Windows 10 Enterprise image that does
> contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to perform
> my tests.
> 
> Additionally, I use an USB stick that I format it with 'sudo mkudffs -b
> 512 --media-type=hd /dev/sdX' and copy some files to it for testing
> 
> BTW, when testing with OVMF AARCH64, I was using my USB stick that
> showed up correctly as 'fsX' in UEFI shell, but with the Windows 10
> Enterprise ISO image, it didn't. I looked at the logs to see what was
> going on and I found out that the emulated CD-ROM drive is reporting a
> block size of 512 instead of 2048 -- which seems wrong to me. With
> 'qemu-system-x86_64 -cdrom', it reports a block size of 2048.
> 
> The Partition driver is still able to find a valid ElTorito partition
> because, regardless the device block size, it always use a logical block
> size of 2048 starting at 32K. In UDF, when searching for AVDPs (Anchor
> Volume Descriptor Pointers), we search for them at fixed locations: 256,
> N - 256, N and 512 -- but, with a block size of 512, the locations
> change to 1024, N - 1024, N and 2048 -- thus breaking the volume
> recognition sequence.
> 
> For testing the Windows image with a block size of 512, I used the
> comformance tool with './udf_test -verbose 60 -blocksize 512
> ~/img/win_ent10.iso' and it failed to find a valid UDF file system. But
> with a block size of 2048, it worked.
> 
> Is there any reason for reporting a block size of 512 when using
> '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing
> something here?
> 
> Thanks!
> Paulo
> 
> [1] - https://www.lscdweb.com/registered/udf_verifier.html
> 
> >
> >
> > Steven Shi
> > Intel\SSG\STO\UEFI Firmware
> >
> > Tel: +86 021-61166522
> > iNet: 821-6522
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Sunday, September 10, 2017 9:52 PM
> >> To: Shi, Steven <steven.shi@intel.com>; edk2-devel-01 <edk2-
> >> devel@lists.01.org>
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Zeng,
> >> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
> >>
> >> Hi Steven,
> >>
> >> On 09/10/17 10:38, Laszlo Ersek wrote:
> >>> On 09/10/17 06:24, Shi, Steven wrote:
> >>>> Hi Laszlo,
> >>>> How could we configure the Qemu and test the UDF driver on OVMF?
> >>>
> >>> I guess you would format e.g. a DVD image with UDF, and attach it to
> >>> QEMU like any other CD-ROM.
> >>
> >> I tried to look into this -- I tried several things, but nothing
> >> produced an UDF image file that, when attached to the VM, would show
> up
> >> in the UEFI shell as FSn:
> >>
> >> Google returned a bunch of pages, but all I found was:
> >> - tips that didn't work (see above),
> >> - confused users (like me) looking for solutions.
> >>
> >> So, at the moment, I have no idea how authoring UDF DVD images is
> >> possible on Linux, so that they'd be recognized in edk2.
> >>
> >> (I'm interested in the edk2 UDF driver not because I want to "author"
> >> UDF DVD images (ISO9660+ElTorito works just fine), but because some
> >> optical media images that were given to me are formatted UDF-only.
> They
> >> can be translated into ISO9660+ElTorito off-line, but that's a chore.)
> >>
> >> Thanks,
> >> Laszlo
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Ni, Ruiyu]

Steven,
Thanks for capturing the bug using ASAN!

-----Original Message-----
From: Shi, Steven 
Sent: Monday, September 11, 2017 9:08 PM
To: Paulo Alcantara <pcacjr@zytor.com>; Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: RE: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

Hi Laszlo,
Your code is good. But there is a "Null Pointer Use" issue, which is a undefined behavior in C spec, in edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:696, column: 7 and line:707, column: 14.

Line695:
  FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
  if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
   ...

The above FileIdentifierDesc can be NULL if you run a .efi in a Udf partition. And the later IS_FID_DIRECTORY_FILE (FileIdentifierDesc) is unsafe if FileIdentifierDesc == NULL. You can test it with below extra code which add debug output when FileIdentifierDesc == NULL, and check the log after load and run something in a Udf partition.

+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -693,6 +693,11 @@ UdfSetPosition (
   PrivFileData = PRIVATE_UDF_FILE_DATA_FROM_THIS (This);

   FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
+  if (FileIdentifierDesc == NULL) {
+    DEBUG ((EFI_D_ERROR, "FileIdentifierDesc == NULL!\n"));
+    return Status;
+  }
+
   if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
     //
     // If the file handle is a directory, the _only_ position that may be set is

I followed the Paulo's suggestion, and found this issue when using the Udf partition of Windows 10 Enterprise ISO image on qemu. Below is my qemu command:
/usr/local/bin/qemu-system-x86_64 -m 5120 -enable-kvm  -machine pc-q35-2.9 -bios OVMF.fd -serial file:serial.log  -cdrom /media/jshi19/My\ Passport/OSImage/ISO/winserver/en_windows_server_2016_x64_dvd_9327751.iso

I've submitted a bug for this issue: https://bugzilla.tianocore.org/show_bug.cgi?id=704. 

I found this issue by edk2 llvm undefined behavior sanitizer, and below is the sanitizer output. FYI.
/home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02B8, column:0x0007 ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called: 
UdfSetPosition
/home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c:696:7
EfiShellFindFilesInDir
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2136:18
ShellSearchHandle
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2413:14
EfiShellFindFiles
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2586:18
EfiShellOpenFileList
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2666:12
ShellOpenFileMetaArg
/home/jshi19/wksp_efi/edk2/ShellPkg/Library/UefiShellLib/UefiShellLib.c:1570:14

/home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02C3, column:0x000E ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called:
...

Steven Shi
Intel\SSG\STO\UEFI Firmware

Tel: +86 021-61166522
iNet: 821-6522

> -----Original Message-----
> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> Sent: Sunday, September 10, 2017 11:52 PM
> To: Shi, Steven <steven.shi@intel.com>; Laszlo Ersek 
> <lersek@redhat.com>;
> edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; 
> Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel 
> <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
> 
> 
> 
> On 10/09/2017 11:27, Shi, Steven wrote:
> > OK. Does the UDF image you created correctly show up as CD-ROM
> content in Linux, e.g Fedora?
> 
> The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso) 
> does not contain a valid UDF file system, so you cannot use it for testing.
> 
> To make sure it really doesn't, I used a "Philips UDF Conformance Tool"
> that you can grab in [1], and the tool didn't find any valid UDF file 
> system in it.
> 
> I've been using an unmodified Windows 10 Enterprise image that does 
> contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to 
> perform my tests.
> 
> Additionally, I use an USB stick that I format it with 'sudo mkudffs 
> -b
> 512 --media-type=hd /dev/sdX' and copy some files to it for testing
> 
> BTW, when testing with OVMF AARCH64, I was using my USB stick that 
> showed up correctly as 'fsX' in UEFI shell, but with the Windows 10 
> Enterprise ISO image, it didn't. I looked at the logs to see what was 
> going on and I found out that the emulated CD-ROM drive is reporting a 
> block size of 512 instead of 2048 -- which seems wrong to me. With
> 'qemu-system-x86_64 -cdrom', it reports a block size of 2048.
> 
> The Partition driver is still able to find a valid ElTorito partition 
> because, regardless the device block size, it always use a logical 
> block size of 2048 starting at 32K. In UDF, when searching for AVDPs 
> (Anchor Volume Descriptor Pointers), we search for them at fixed 
> locations: 256, N - 256, N and 512 -- but, with a block size of 512, 
> the locations change to 1024, N - 1024, N and 2048 -- thus breaking 
> the volume recognition sequence.
> 
> For testing the Windows image with a block size of 512, I used the 
> comformance tool with './udf_test -verbose 60 -blocksize 512 
> ~/img/win_ent10.iso' and it failed to find a valid UDF file system. 
> But with a block size of 2048, it worked.
> 
> Is there any reason for reporting a block size of 512 when using 
> '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing 
> something here?
> 
> Thanks!
> Paulo
> 
> [1] - https://www.lscdweb.com/registered/udf_verifier.html
> 
> >
> >
> > Steven Shi
> > Intel\SSG\STO\UEFI Firmware
> >
> > Tel: +86 021-61166522
> > iNet: 821-6522
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Sunday, September 10, 2017 9:52 PM
> >> To: Shi, Steven <steven.shi@intel.com>; edk2-devel-01 <edk2- 
> >> devel@lists.01.org>
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric 
> >> <eric.dong@intel.com>;
> Zeng,
> >> Star <star.zeng@intel.com>; Ard Biesheuvel 
> >> <ard.biesheuvel@linaro.org>
> >> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and 
> >> cleanups
> >>
> >> Hi Steven,
> >>
> >> On 09/10/17 10:38, Laszlo Ersek wrote:
> >>> On 09/10/17 06:24, Shi, Steven wrote:
> >>>> Hi Laszlo,
> >>>> How could we configure the Qemu and test the UDF driver on OVMF?
> >>>
> >>> I guess you would format e.g. a DVD image with UDF, and attach it 
> >>> to QEMU like any other CD-ROM.
> >>
> >> I tried to look into this -- I tried several things, but nothing 
> >> produced an UDF image file that, when attached to the VM, would 
> >> show
> up
> >> in the UEFI shell as FSn:
> >>
> >> Google returned a bunch of pages, but all I found was:
> >> - tips that didn't work (see above),
> >> - confused users (like me) looking for solutions.
> >>
> >> So, at the moment, I have no idea how authoring UDF DVD images is 
> >> possible on Linux, so that they'd be recognized in edk2.
> >>
> >> (I'm interested in the edk2 UDF driver not because I want to "author"
> >> UDF DVD images (ISO9660+ElTorito works just fine), but because some 
> >> optical media images that were given to me are formatted UDF-only.
> They
> >> can be translated into ISO9660+ElTorito off-line, but that's a 
> >> chore.)
> >>
> >> Thanks,
> >> Laszlo
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Laszlo Ersek]

On 09/11/17 15:07, Shi, Steven wrote:
> Hi Laszlo,
> Your code is good. But there is a "Null Pointer Use" issue, which is a undefined behavior in C spec, in edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:696, column: 7 and line:707, column: 14.
> 
> Line695:
>   FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
>   if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
>    ...
> 
> The above FileIdentifierDesc can be NULL if you run a .efi in a Udf partition. And the later IS_FID_DIRECTORY_FILE (FileIdentifierDesc) is unsafe if FileIdentifierDesc == NULL. You can test it with below extra code which add debug output when FileIdentifierDesc == NULL, and check the log after load and run something in a Udf partition.
> 
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -693,6 +693,11 @@ UdfSetPosition (
>    PrivFileData = PRIVATE_UDF_FILE_DATA_FROM_THIS (This);
> 
>    FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
> +  if (FileIdentifierDesc == NULL) {
> +    DEBUG ((EFI_D_ERROR, "FileIdentifierDesc == NULL!\n"));
> +    return Status;
> +  }
> +
>    if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
>      //
>      // If the file handle is a directory, the _only_ position that may be set is
> 
> I followed the Paulo's suggestion, and found this issue when using the Udf partition of Windows 10 Enterprise ISO image on qemu. Below is my qemu command:
> /usr/local/bin/qemu-system-x86_64 -m 5120 -enable-kvm  -machine pc-q35-2.9 -bios OVMF.fd -serial file:serial.log  -cdrom /media/jshi19/My\ Passport/OSImage/ISO/winserver/en_windows_server_2016_x64_dvd_9327751.iso
> 
> I've submitted a bug for this issue: https://bugzilla.tianocore.org/show_bug.cgi?id=704.

Right, I saw the report (via
<https://lists.01.org/mailman/listinfo/edk2-bugs>), thank you for filing it.

I asked Paulo a few minutes ago to register in the TianoCore Bugzilla,
and to take the BZ.

I don't intend to co-maintain UdfDxe permanently, aside from one-off
patches and reviews, exactly like with any other MdeModulePkg driver.
I'm totally unfamiliar with UdfDxe code, I'm just interested in using
the feature. The only reason I sent these five patches over the weekend
is that the initial code drop, technically committed by me, broke the
build with CLANG38, and Paulo wasn't near his computer. I wanted to fix
the build breakage ASAP.

If you find other issues, please continue filing BZs, and Paulo should
please continue helping out with them.

Thanks!
Laszlo

> 
> I found this issue by edk2 llvm undefined behavior sanitizer, and below is the sanitizer output. FYI.
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02B8, column:0x0007 ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
> ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called: 
> UdfSetPosition
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c:696:7
> EfiShellFindFilesInDir
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2136:18
> ShellSearchHandle
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2413:14
> EfiShellFindFiles
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2586:18
> EfiShellOpenFileList
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2666:12
> ShellOpenFileMetaArg
> /home/jshi19/wksp_efi/edk2/ShellPkg/Library/UefiShellLib/UefiShellLib.c:1570:14
> 
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02C3, column:0x000E ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
> ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called:
> ...
> 
> Steven Shi
> Intel\SSG\STO\UEFI Firmware
> 
> Tel: +86 021-61166522
> iNet: 821-6522
> 
>> -----Original Message-----
>> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>> Sent: Sunday, September 10, 2017 11:52 PM
>> To: Shi, Steven <steven.shi@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>> edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng,
>> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
>>
>>
>>
>> On 10/09/2017 11:27, Shi, Steven wrote:
>>> OK. Does the UDF image you created correctly show up as CD-ROM
>> content in Linux, e.g Fedora?
>>
>> The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso) does
>> not contain a valid UDF file system, so you cannot use it for testing.
>>
>> To make sure it really doesn't, I used a "Philips UDF Conformance Tool"
>> that you can grab in [1], and the tool didn't find any valid UDF file
>> system in it.
>>
>> I've been using an unmodified Windows 10 Enterprise image that does
>> contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to perform
>> my tests.
>>
>> Additionally, I use an USB stick that I format it with 'sudo mkudffs -b
>> 512 --media-type=hd /dev/sdX' and copy some files to it for testing
>>
>> BTW, when testing with OVMF AARCH64, I was using my USB stick that
>> showed up correctly as 'fsX' in UEFI shell, but with the Windows 10
>> Enterprise ISO image, it didn't. I looked at the logs to see what was
>> going on and I found out that the emulated CD-ROM drive is reporting a
>> block size of 512 instead of 2048 -- which seems wrong to me. With
>> 'qemu-system-x86_64 -cdrom', it reports a block size of 2048.
>>
>> The Partition driver is still able to find a valid ElTorito partition
>> because, regardless the device block size, it always use a logical block
>> size of 2048 starting at 32K. In UDF, when searching for AVDPs (Anchor
>> Volume Descriptor Pointers), we search for them at fixed locations: 256,
>> N - 256, N and 512 -- but, with a block size of 512, the locations
>> change to 1024, N - 1024, N and 2048 -- thus breaking the volume
>> recognition sequence.
>>
>> For testing the Windows image with a block size of 512, I used the
>> comformance tool with './udf_test -verbose 60 -blocksize 512
>> ~/img/win_ent10.iso' and it failed to find a valid UDF file system. But
>> with a block size of 2048, it worked.
>>
>> Is there any reason for reporting a block size of 512 when using
>> '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing
>> something here?
>>
>> Thanks!
>> Paulo
>>
>> [1] - https://www.lscdweb.com/registered/udf_verifier.html
>>
>>>
>>>
>>> Steven Shi
>>> Intel\SSG\STO\UEFI Firmware
>>>
>>> Tel: +86 021-61166522
>>> iNet: 821-6522
>>>
>>>> -----Original Message-----
>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>> Sent: Sunday, September 10, 2017 9:52 PM
>>>> To: Shi, Steven <steven.shi@intel.com>; edk2-devel-01 <edk2-
>>>> devel@lists.01.org>
>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
>> Zeng,
>>>> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
>>>>
>>>> Hi Steven,
>>>>
>>>> On 09/10/17 10:38, Laszlo Ersek wrote:
>>>>> On 09/10/17 06:24, Shi, Steven wrote:
>>>>>> Hi Laszlo,
>>>>>> How could we configure the Qemu and test the UDF driver on OVMF?
>>>>>
>>>>> I guess you would format e.g. a DVD image with UDF, and attach it to
>>>>> QEMU like any other CD-ROM.
>>>>
>>>> I tried to look into this -- I tried several things, but nothing
>>>> produced an UDF image file that, when attached to the VM, would show
>> up
>>>> in the UEFI shell as FSn:
>>>>
>>>> Google returned a bunch of pages, but all I found was:
>>>> - tips that didn't work (see above),
>>>> - confused users (like me) looking for solutions.
>>>>
>>>> So, at the moment, I have no idea how authoring UDF DVD images is
>>>> possible on Linux, so that they'd be recognized in edk2.
>>>>
>>>> (I'm interested in the edk2 UDF driver not because I want to "author"
>>>> UDF DVD images (ISO9660+ElTorito works just fine), but because some
>>>> optical media images that were given to me are formatted UDF-only.
>> They
>>>> can be translated into ISO9660+ElTorito off-line, but that's a chore.)
>>>>
>>>> Thanks,
>>>> Laszlo
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Paulo Alcantara]

Hi Steven,

On 11/09/2017 10:07, Shi, Steven wrote:
> Hi Laszlo,
> Your code is good. But there is a "Null Pointer Use" issue, which is a undefined behavior in C spec, in edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:696, column: 7 and line:707, column: 14.
> 
> Line695:
>    FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
>    if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
>     ...
> 
> The above FileIdentifierDesc can be NULL if you run a .efi in a Udf partition. And the later IS_FID_DIRECTORY_FILE (FileIdentifierDesc) is unsafe if FileIdentifierDesc == NULL. You can test it with below extra code which add debug output when FileIdentifierDesc == NULL, and check the log after load and run something in a Udf partition.
> 
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -693,6 +693,11 @@ UdfSetPosition (
>     PrivFileData = PRIVATE_UDF_FILE_DATA_FROM_THIS (This);
> 
>     FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
> +  if (FileIdentifierDesc == NULL) {
> +    DEBUG ((EFI_D_ERROR, "FileIdentifierDesc == NULL!\n"));
> +    return Status;
> +  }
> +
>     if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
>       //
>       // If the file handle is a directory, the _only_ position that may be set is
> 
> I followed the Paulo's suggestion, and found this issue when using the Udf partition of Windows 10 Enterprise ISO image on qemu. Below is my qemu command:
> /usr/local/bin/qemu-system-x86_64 -m 5120 -enable-kvm  -machine pc-q35-2.9 -bios OVMF.fd -serial file:serial.log  -cdrom /media/jshi19/My\ Passport/OSImage/ISO/winserver/en_windows_server_2016_x64_dvd_9327751.iso
> 
> I've submitted a bug for this issue: https://bugzilla.tianocore.org/show_bug.cgi?id=704.
> 
> I found this issue by edk2 llvm undefined behavior sanitizer, and below is the sanitizer output. FYI.
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02B8, column:0x0007 ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
> ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called:
> UdfSetPosition
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c:696:7
> EfiShellFindFilesInDir
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2136:18
> ShellSearchHandle
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2413:14
> EfiShellFindFiles
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2586:18
> EfiShellOpenFileList
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2666:12
> ShellOpenFileMetaArg
> /home/jshi19/wksp_efi/edk2/ShellPkg/Library/UefiShellLib/UefiShellLib.c:1570:14
> 
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02C3, column:0x000E ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
> ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called:
> ...

Thank you for catching this bug up. I think this issue is related to 
accessing a File Identifier Descriptor from a root directory file -- 
e.g., this should be "FileIdentifierDesc = 
PrivFileData->Root->FileIdentifierDesc;"

Could you please replace the broken assignment with:

FileIdentifierDesc = _FILE(PrivFileData)->FileIdentifierDesc;
ASSERT (FileIdentifierDesc != NULL);
...

Tell me if that worked for you. If so, I could send a formal patch 
fixing this issue later.

(BTW, I do apologize the late replies but I'm only able to work on this 
in my free time)

Thanks,
Paulo

> 
> Steven Shi
> Intel\SSG\STO\UEFI Firmware
> 
> Tel: +86 021-61166522
> iNet: 821-6522
> 
>> -----Original Message-----
>> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>> Sent: Sunday, September 10, 2017 11:52 PM
>> To: Shi, Steven <steven.shi@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>> edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng,
>> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
>>
>>
>>
>> On 10/09/2017 11:27, Shi, Steven wrote:
>>> OK. Does the UDF image you created correctly show up as CD-ROM
>> content in Linux, e.g Fedora?
>>
>> The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso) does
>> not contain a valid UDF file system, so you cannot use it for testing.
>>
>> To make sure it really doesn't, I used a "Philips UDF Conformance Tool"
>> that you can grab in [1], and the tool didn't find any valid UDF file
>> system in it.
>>
>> I've been using an unmodified Windows 10 Enterprise image that does
>> contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to perform
>> my tests.
>>
>> Additionally, I use an USB stick that I format it with 'sudo mkudffs -b
>> 512 --media-type=hd /dev/sdX' and copy some files to it for testing
>>
>> BTW, when testing with OVMF AARCH64, I was using my USB stick that
>> showed up correctly as 'fsX' in UEFI shell, but with the Windows 10
>> Enterprise ISO image, it didn't. I looked at the logs to see what was
>> going on and I found out that the emulated CD-ROM drive is reporting a
>> block size of 512 instead of 2048 -- which seems wrong to me. With
>> 'qemu-system-x86_64 -cdrom', it reports a block size of 2048.
>>
>> The Partition driver is still able to find a valid ElTorito partition
>> because, regardless the device block size, it always use a logical block
>> size of 2048 starting at 32K. In UDF, when searching for AVDPs (Anchor
>> Volume Descriptor Pointers), we search for them at fixed locations: 256,
>> N - 256, N and 512 -- but, with a block size of 512, the locations
>> change to 1024, N - 1024, N and 2048 -- thus breaking the volume
>> recognition sequence.
>>
>> For testing the Windows image with a block size of 512, I used the
>> comformance tool with './udf_test -verbose 60 -blocksize 512
>> ~/img/win_ent10.iso' and it failed to find a valid UDF file system. But
>> with a block size of 2048, it worked.
>>
>> Is there any reason for reporting a block size of 512 when using
>> '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing
>> something here?
>>
>> Thanks!
>> Paulo
>>
>> [1] - https://www.lscdweb.com/registered/udf_verifier.html
>>
>>>
>>>
>>> Steven Shi
>>> Intel\SSG\STO\UEFI Firmware
>>>
>>> Tel: +86 021-61166522
>>> iNet: 821-6522
>>>
>>>> -----Original Message-----
>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>> Sent: Sunday, September 10, 2017 9:52 PM
>>>> To: Shi, Steven <steven.shi@intel.com>; edk2-devel-01 <edk2-
>>>> devel@lists.01.org>
>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
>> Zeng,
>>>> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
>>>>
>>>> Hi Steven,
>>>>
>>>> On 09/10/17 10:38, Laszlo Ersek wrote:
>>>>> On 09/10/17 06:24, Shi, Steven wrote:
>>>>>> Hi Laszlo,
>>>>>> How could we configure the Qemu and test the UDF driver on OVMF?
>>>>>
>>>>> I guess you would format e.g. a DVD image with UDF, and attach it to
>>>>> QEMU like any other CD-ROM.
>>>>
>>>> I tried to look into this -- I tried several things, but nothing
>>>> produced an UDF image file that, when attached to the VM, would show
>> up
>>>> in the UEFI shell as FSn:
>>>>
>>>> Google returned a bunch of pages, but all I found was:
>>>> - tips that didn't work (see above),
>>>> - confused users (like me) looking for solutions.
>>>>
>>>> So, at the moment, I have no idea how authoring UDF DVD images is
>>>> possible on Linux, so that they'd be recognized in edk2.
>>>>
>>>> (I'm interested in the edk2 UDF driver not because I want to "author"
>>>> UDF DVD images (ISO9660+ElTorito works just fine), but because some
>>>> optical media images that were given to me are formatted UDF-only.
>> They
>>>> can be translated into ISO9660+ElTorito off-line, but that's a chore.)
>>>>
>>>> Thanks,
>>>> Laszlo
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
> 

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Paulo Alcantara]

On 11/09/2017 03:58, Laszlo Ersek wrote:
> On 09/10/17 17:51, Paulo Alcantara wrote:
>>
>>
>> On 10/09/2017 11:27, Shi, Steven wrote:
>>> OK. Does the UDF image you created correctly show up as CD-ROM content
>>> in Linux, e.g Fedora?
>>
>> The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso) does
>> not contain a valid UDF file system, so you cannot use it for testing.
>>
>> To make sure it really doesn't, I used a "Philips UDF Conformance Tool"
>> that you can grab in [1], and the tool didn't find any valid UDF file
>> system in it.
>>
>> I've been using an unmodified Windows 10 Enterprise image that does
>> contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to perform
>> my tests.
>>
>> Additionally, I use an USB stick that I format it with 'sudo mkudffs -b
>> 512 --media-type=hd /dev/sdX' and copy some files to it for testing
>>
>> BTW, when testing with OVMF AARCH64, I was using my USB stick that
>> showed up correctly as 'fsX' in UEFI shell, but with the Windows 10
>> Enterprise ISO image, it didn't. I looked at the logs to see what was
>> going on and I found out that the emulated CD-ROM drive is reporting a
>> block size of 512 instead of 2048 -- which seems wrong to me. With
>> 'qemu-system-x86_64 -cdrom', it reports a block size of 2048.
>>
>> The Partition driver is still able to find a valid ElTorito partition
>> because, regardless the device block size, it always use a logical block
>> size of 2048 starting at 32K. In UDF, when searching for AVDPs (Anchor
>> Volume Descriptor Pointers), we search for them at fixed locations: 256,
>> N - 256, N and 512 -- but, with a block size of 512, the locations
>> change to 1024, N - 1024, N and 2048 -- thus breaking the volume
>> recognition sequence.
>>
>> For testing the Windows image with a block size of 512, I used the
>> comformance tool with './udf_test -verbose 60 -blocksize 512
>> ~/img/win_ent10.iso' and it failed to find a valid UDF file system. But
>> with a block size of 2048, it worked.
>>
>> Is there any reason for reporting a block size of 512 when using
>> '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing
>> something here?
> 
> "-cdrom" is a legacy QEMU option, a shorthand that expands to a -drive
> option, and at least one -device option (it could expand to multiple
> -device options). And, this expansion can be different on different
> machine types. On i440fx machine types, -cdrom can mean an IDE CD, on
> q35, an AHCI CD, an aarch64/virt, a virtio-scsi-pci controller and a
> scsi-cd. For this reason, it is always best to use complete -drive and
> -device specifications (which is equivalent to saying it is best to
> always use libvirt).
> 
> Now, the above does not imlpy that no bug can exist in this space. Can
> you run the "info qtree" monitor command, in both cases, and compare the
> output with regard to the CD-ROM?

OK. Thanks for clarifying that to me. I'll do it when I have a chance 
and tell you.

Paulo

> 
> Thanks
> Laszlo
> 
>> Thanks!
>> Paulo
>>
>> [1] - https://www.lscdweb.com/registered/udf_verifier.html

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Shi, Steven]

Hi Paulo,
Your fix works for me. The "Null Pointer Use" issue disappears after apply your new assignment patch. Thanks!


Steven Shi
Intel\SSG\STO\UEFI Firmware

Tel: +86 021-61166522
iNet: 821-6522


> -----Original Message-----
> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> Sent: Monday, September 11, 2017 9:52 PM
> To: Shi, Steven <steven.shi@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
> 
> Hi Steven,
> 
> On 11/09/2017 10:07, Shi, Steven wrote:
> > Hi Laszlo,
> > Your code is good. But there is a "Null Pointer Use" issue, which is a
> undefined behavior in C spec, in
> edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:696, column: 7 and
> line:707, column: 14.
> >
> > Line695:
> >    FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
> >    if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
> >     ...
> >
> > The above FileIdentifierDesc can be NULL if you run a .efi in a Udf partition.
> And the later IS_FID_DIRECTORY_FILE (FileIdentifierDesc) is unsafe if
> FileIdentifierDesc == NULL. You can test it with below extra code which add
> debug output when FileIdentifierDesc == NULL, and check the log after load
> and run something in a Udf partition.
> >
> > +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> > @@ -693,6 +693,11 @@ UdfSetPosition (
> >     PrivFileData = PRIVATE_UDF_FILE_DATA_FROM_THIS (This);
> >
> >     FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
> > +  if (FileIdentifierDesc == NULL) {
> > +    DEBUG ((EFI_D_ERROR, "FileIdentifierDesc == NULL!\n"));
> > +    return Status;
> > +  }
> > +
> >     if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
> >       //
> >       // If the file handle is a directory, the _only_ position that may be set is
> >
> > I followed the Paulo's suggestion, and found this issue when using the Udf
> partition of Windows 10 Enterprise ISO image on qemu. Below is my qemu
> command:
> > /usr/local/bin/qemu-system-x86_64 -m 5120 -enable-kvm  -machine pc-
> q35-2.9 -bios OVMF.fd -serial file:serial.log  -cdrom /media/jshi19/My\
> Passport/OSImage/ISO/winserver/en_windows_server_2016_x64_dvd_9327
> 751.iso
> >
> > I've submitted a bug for this issue:
> https://bugzilla.tianocore.org/show_bug.cgi?id=704.
> >
> > I found this issue by edk2 llvm undefined behavior sanitizer, and below is
> the sanitizer output. FYI.
> >
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c,
> line:0x02B8, column:0x0007 ErrorType = NullPointerUse: member access
> within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
> > ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is
> called:
> > UdfSetPosition
> >
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c:
> 696:7
> > EfiShellFindFilesInDir
> >
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:213
> 6:18
> > ShellSearchHandle
> >
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:241
> 3:14
> > EfiShellFindFiles
> >
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:258
> 6:18
> > EfiShellOpenFileList
> >
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:266
> 6:12
> > ShellOpenFileMetaArg
> >
> /home/jshi19/wksp_efi/edk2/ShellPkg/Library/UefiShellLib/UefiShellLib.c:15
> 70:14
> >
> >
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c,
> line:0x02C3, column:0x000E ErrorType = NullPointerUse: member access
> within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
> > ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is
> called:
> > ...
> 
> Thank you for catching this bug up. I think this issue is related to
> accessing a File Identifier Descriptor from a root directory file --
> e.g., this should be "FileIdentifierDesc =
> PrivFileData->Root->FileIdentifierDesc;"
> 
> Could you please replace the broken assignment with:
> 
> FileIdentifierDesc = _FILE(PrivFileData)->FileIdentifierDesc;
> ASSERT (FileIdentifierDesc != NULL);
> ...
> 
> Tell me if that worked for you. If so, I could send a formal patch
> fixing this issue later.
> 
> (BTW, I do apologize the late replies but I'm only able to work on this
> in my free time)
> 
> Thanks,
> Paulo
> 
> >
> > Steven Shi
> > Intel\SSG\STO\UEFI Firmware
> >
> > Tel: +86 021-61166522
> > iNet: 821-6522
> >
> >> -----Original Message-----
> >> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> >> Sent: Sunday, September 10, 2017 11:52 PM
> >> To: Shi, Steven <steven.shi@intel.com>; Laszlo Ersek
> <lersek@redhat.com>;
> >> edk2-devel-01 <edk2-devel@lists.01.org>
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Zeng,
> >> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
> >>
> >>
> >>
> >> On 10/09/2017 11:27, Shi, Steven wrote:
> >>> OK. Does the UDF image you created correctly show up as CD-ROM
> >> content in Linux, e.g Fedora?
> >>
> >> The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso)
> does
> >> not contain a valid UDF file system, so you cannot use it for testing.
> >>
> >> To make sure it really doesn't, I used a "Philips UDF Conformance Tool"
> >> that you can grab in [1], and the tool didn't find any valid UDF file
> >> system in it.
> >>
> >> I've been using an unmodified Windows 10 Enterprise image that does
> >> contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to perform
> >> my tests.
> >>
> >> Additionally, I use an USB stick that I format it with 'sudo mkudffs -b
> >> 512 --media-type=hd /dev/sdX' and copy some files to it for testing
> >>
> >> BTW, when testing with OVMF AARCH64, I was using my USB stick that
> >> showed up correctly as 'fsX' in UEFI shell, but with the Windows 10
> >> Enterprise ISO image, it didn't. I looked at the logs to see what was
> >> going on and I found out that the emulated CD-ROM drive is reporting a
> >> block size of 512 instead of 2048 -- which seems wrong to me. With
> >> 'qemu-system-x86_64 -cdrom', it reports a block size of 2048.
> >>
> >> The Partition driver is still able to find a valid ElTorito partition
> >> because, regardless the device block size, it always use a logical block
> >> size of 2048 starting at 32K. In UDF, when searching for AVDPs (Anchor
> >> Volume Descriptor Pointers), we search for them at fixed locations: 256,
> >> N - 256, N and 512 -- but, with a block size of 512, the locations
> >> change to 1024, N - 1024, N and 2048 -- thus breaking the volume
> >> recognition sequence.
> >>
> >> For testing the Windows image with a block size of 512, I used the
> >> comformance tool with './udf_test -verbose 60 -blocksize 512
> >> ~/img/win_ent10.iso' and it failed to find a valid UDF file system. But
> >> with a block size of 2048, it worked.
> >>
> >> Is there any reason for reporting a block size of 512 when using
> >> '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing
> >> something here?
> >>
> >> Thanks!
> >> Paulo
> >>
> >> [1] - https://www.lscdweb.com/registered/udf_verifier.html
> >>
> >>>
> >>>
> >>> Steven Shi
> >>> Intel\SSG\STO\UEFI Firmware
> >>>
> >>> Tel: +86 021-61166522
> >>> iNet: 821-6522
> >>>
> >>>> -----Original Message-----
> >>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >>>> Sent: Sunday, September 10, 2017 9:52 PM
> >>>> To: Shi, Steven <steven.shi@intel.com>; edk2-devel-01 <edk2-
> >>>> devel@lists.01.org>
> >>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
> >> Zeng,
> >>>> Star <star.zeng@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> >>>> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and
> cleanups
> >>>>
> >>>> Hi Steven,
> >>>>
> >>>> On 09/10/17 10:38, Laszlo Ersek wrote:
> >>>>> On 09/10/17 06:24, Shi, Steven wrote:
> >>>>>> Hi Laszlo,
> >>>>>> How could we configure the Qemu and test the UDF driver on OVMF?
> >>>>>
> >>>>> I guess you would format e.g. a DVD image with UDF, and attach it to
> >>>>> QEMU like any other CD-ROM.
> >>>>
> >>>> I tried to look into this -- I tried several things, but nothing
> >>>> produced an UDF image file that, when attached to the VM, would
> show
> >> up
> >>>> in the UEFI shell as FSn:
> >>>>
> >>>> Google returned a bunch of pages, but all I found was:
> >>>> - tips that didn't work (see above),
> >>>> - confused users (like me) looking for solutions.
> >>>>
> >>>> So, at the moment, I have no idea how authoring UDF DVD images is
> >>>> possible on Linux, so that they'd be recognized in edk2.
> >>>>
> >>>> (I'm interested in the edk2 UDF driver not because I want to "author"
> >>>> UDF DVD images (ISO9660+ElTorito works just fine), but because some
> >>>> optical media images that were given to me are formatted UDF-only.
> >> They
> >>>> can be translated into ISO9660+ElTorito off-line, but that's a chore.)
> >>>>
> >>>> Thanks,
> >>>> Laszlo
> >>> _______________________________________________
> >>> edk2-devel mailing list
> >>> edk2-devel@lists.01.org
> >>> https://lists.01.org/mailman/listinfo/edk2-devel
> >>>
> >

Re: [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators

[Bi, Dandan]

Hi Laszlo,

When do you plan to push this patch? IA32 build is blocked for this issue now.

Thanks,
Dandan

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Sunday, September 10, 2017 8:13 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: [edk2] [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators

In edk2, the division and shifting of 64-bit values are forbidden with C-language operators, because the compiler may generate intrinsic calls for them.

For example, clang-3.8 emits a call to "__umoddi3" for

  UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize

in PartitionInstallUdfChildHandles(), if PartitionDxe is built for IA32, which then fails to link.

UDF_LOGICAL_SECTOR_SIZE has type UINT64, while EFI_BLOCK_IO_MEDIA.BlockSize has type UINT32(). Replace the % operator with a DivU64x32Remainder() call.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
index c1d44809bfd2..c491ef25f47e 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
@@ -234,10 +234,11 @@ PartitionInstallUdfChildHandles (
   IN  EFI_BLOCK_IO_PROTOCOL        *BlockIo,
   IN  EFI_BLOCK_IO2_PROTOCOL       *BlockIo2,
   IN  EFI_DEVICE_PATH_PROTOCOL     *DevicePath
   )
 {
+  UINT32                       RemainderByMediaBlockSize;
   EFI_STATUS                   Status;
   EFI_BLOCK_IO_MEDIA           *Media;
   EFI_DEVICE_PATH_PROTOCOL     *DevicePathNode;
   EFI_GUID                     *VendorDefinedGuid;
   EFI_GUID                     UdfDevPathGuid = EFI_UDF_DEVICE_PATH_GUID;
@@ -246,11 +247,16 @@ PartitionInstallUdfChildHandles (
   Media = BlockIo->Media;
 
   //
   // Check if UDF logical block size is multiple of underlying device block size
   //
-  if ((UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize) != 0 ||
+  DivU64x32Remainder (
+    UDF_LOGICAL_SECTOR_SIZE,   // Dividend
+    Media->BlockSize,          // Divisor
+    &RemainderByMediaBlockSize // Remainder
+    );
+  if (RemainderByMediaBlockSize != 0 ||
       Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE) {
     return EFI_NOT_FOUND;
   }
 
   DevicePathNode = DevicePath;
--
2.14.1.3.gb7cf6e02401b


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators

[Laszlo Ersek]

On 09/12/17 07:41, Bi, Dandan wrote:
> Hi Laszlo,
> 
> When do you plan to push this patch? IA32 build is blocked for this issue now.

I was ready to push the series yesterday; I just hoped I'd get review
feedback from MdeModulePkg maintainers as well, and/or from Ray, in one
or two days.

These are strongly localized changes that require no knowledge of the
UDF driver. (I don't have that knowledge myself, to begin with.) At
least an Acked-by would be nice.

If someone from Intel tells me I can push this with the R-b's that are
currently on the list, I'm totally game.

Thanks!
Laszlo

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Sunday, September 10, 2017 8:13 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [edk2] [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators
> 
> In edk2, the division and shifting of 64-bit values are forbidden with C-language operators, because the compiler may generate intrinsic calls for them.
> 
> For example, clang-3.8 emits a call to "__umoddi3" for
> 
>   UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize
> 
> in PartitionInstallUdfChildHandles(), if PartitionDxe is built for IA32, which then fails to link.
> 
> UDF_LOGICAL_SECTOR_SIZE has type UINT64, while EFI_BLOCK_IO_MEDIA.BlockSize has type UINT32(). Replace the % operator with a DivU64x32Remainder() call.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Paulo Alcantara <pcacjr@zytor.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> index c1d44809bfd2..c491ef25f47e 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> @@ -234,10 +234,11 @@ PartitionInstallUdfChildHandles (
>    IN  EFI_BLOCK_IO_PROTOCOL        *BlockIo,
>    IN  EFI_BLOCK_IO2_PROTOCOL       *BlockIo2,
>    IN  EFI_DEVICE_PATH_PROTOCOL     *DevicePath
>    )
>  {
> +  UINT32                       RemainderByMediaBlockSize;
>    EFI_STATUS                   Status;
>    EFI_BLOCK_IO_MEDIA           *Media;
>    EFI_DEVICE_PATH_PROTOCOL     *DevicePathNode;
>    EFI_GUID                     *VendorDefinedGuid;
>    EFI_GUID                     UdfDevPathGuid = EFI_UDF_DEVICE_PATH_GUID;
> @@ -246,11 +247,16 @@ PartitionInstallUdfChildHandles (
>    Media = BlockIo->Media;
>  
>    //
>    // Check if UDF logical block size is multiple of underlying device block size
>    //
> -  if ((UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize) != 0 ||
> +  DivU64x32Remainder (
> +    UDF_LOGICAL_SECTOR_SIZE,   // Dividend
> +    Media->BlockSize,          // Divisor
> +    &RemainderByMediaBlockSize // Remainder
> +    );
> +  if (RemainderByMediaBlockSize != 0 ||
>        Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE) {
>      return EFI_NOT_FOUND;
>    }
>  
>    DevicePathNode = DevicePath;
> --
> 2.14.1.3.gb7cf6e02401b
> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators

[Ni, Ruiyu]

Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

Thanks/Ray

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, September 12, 2017 3:59 PM
> To: Bi, Dandan <dandan.bi@intel.com>; edk2-devel-01 <edk2-
> devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide
> 64-bit values with C operators
> 
> On 09/12/17 07:41, Bi, Dandan wrote:
> > Hi Laszlo,
> >
> > When do you plan to push this patch? IA32 build is blocked for this issue
> now.
> 
> I was ready to push the series yesterday; I just hoped I'd get review
> feedback from MdeModulePkg maintainers as well, and/or from Ray, in one
> or two days.
> 
> These are strongly localized changes that require no knowledge of the UDF
> driver. (I don't have that knowledge myself, to begin with.) At least an
> Acked-by would be nice.
> 
> If someone from Intel tells me I can push this with the R-b's that are currently
> on the list, I'm totally game.
> 
> Thanks!
> Laszlo
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Laszlo Ersek
> > Sent: Sunday, September 10, 2017 8:13 AM
> > To: edk2-devel-01 <edk2-devel@lists.01.org>
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
> > Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>
> > Subject: [edk2] [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide
> > 64-bit values with C operators
> >
> > In edk2, the division and shifting of 64-bit values are forbidden with C-
> language operators, because the compiler may generate intrinsic calls for
> them.
> >
> > For example, clang-3.8 emits a call to "__umoddi3" for
> >
> >   UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize
> >
> > in PartitionInstallUdfChildHandles(), if PartitionDxe is built for IA32, which
> then fails to link.
> >
> > UDF_LOGICAL_SECTOR_SIZE has type UINT64, while
> EFI_BLOCK_IO_MEDIA.BlockSize has type UINT32(). Replace the % operator
> with a DivU64x32Remainder() call.
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Paulo Alcantara <pcacjr@zytor.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> > b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> > index c1d44809bfd2..c491ef25f47e 100644
> > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> > @@ -234,10 +234,11 @@ PartitionInstallUdfChildHandles (
> >    IN  EFI_BLOCK_IO_PROTOCOL        *BlockIo,
> >    IN  EFI_BLOCK_IO2_PROTOCOL       *BlockIo2,
> >    IN  EFI_DEVICE_PATH_PROTOCOL     *DevicePath
> >    )
> >  {
> > +  UINT32                       RemainderByMediaBlockSize;
> >    EFI_STATUS                   Status;
> >    EFI_BLOCK_IO_MEDIA           *Media;
> >    EFI_DEVICE_PATH_PROTOCOL     *DevicePathNode;
> >    EFI_GUID                     *VendorDefinedGuid;
> >    EFI_GUID                     UdfDevPathGuid = EFI_UDF_DEVICE_PATH_GUID;
> > @@ -246,11 +247,16 @@ PartitionInstallUdfChildHandles (
> >    Media = BlockIo->Media;
> >
> >    //
> >    // Check if UDF logical block size is multiple of underlying device block size
> >    //
> > -  if ((UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize) != 0 ||
> > +  DivU64x32Remainder (
> > +    UDF_LOGICAL_SECTOR_SIZE,   // Dividend
> > +    Media->BlockSize,          // Divisor
> > +    &RemainderByMediaBlockSize // Remainder
> > +    );
> > +  if (RemainderByMediaBlockSize != 0 ||
> >        Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE) {
> >      return EFI_NOT_FOUND;
> >    }
> >
> >    DevicePathNode = DevicePath;
> > --
> > 2.14.1.3.gb7cf6e02401b
> >
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >

Re: [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

I am not sure whether the other patches in this series has been reviewed or not.
Since this patch is fixing build break, I think we can have this patch pushed first after reviewed. And really appreciate that. :)

Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, September 12, 2017 3:59 PM
To: Bi, Dandan <dandan.bi@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators

On 09/12/17 07:41, Bi, Dandan wrote:
> Hi Laszlo,
> 
> When do you plan to push this patch? IA32 build is blocked for this issue now.

I was ready to push the series yesterday; I just hoped I'd get review feedback from MdeModulePkg maintainers as well, and/or from Ray, in one or two days.

These are strongly localized changes that require no knowledge of the UDF driver. (I don't have that knowledge myself, to begin with.) At least an Acked-by would be nice.

If someone from Intel tells me I can push this with the R-b's that are currently on the list, I'm totally game.

Thanks!
Laszlo

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Laszlo Ersek
> Sent: Sunday, September 10, 2017 8:13 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; 
> Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel 
> <ard.biesheuvel@linaro.org>
> Subject: [edk2] [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 
> 64-bit values with C operators
> 
> In edk2, the division and shifting of 64-bit values are forbidden with C-language operators, because the compiler may generate intrinsic calls for them.
> 
> For example, clang-3.8 emits a call to "__umoddi3" for
> 
>   UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize
> 
> in PartitionInstallUdfChildHandles(), if PartitionDxe is built for IA32, which then fails to link.
> 
> UDF_LOGICAL_SECTOR_SIZE has type UINT64, while EFI_BLOCK_IO_MEDIA.BlockSize has type UINT32(). Replace the % operator with a DivU64x32Remainder() call.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Paulo Alcantara <pcacjr@zytor.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c 
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> index c1d44809bfd2..c491ef25f47e 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> @@ -234,10 +234,11 @@ PartitionInstallUdfChildHandles (
>    IN  EFI_BLOCK_IO_PROTOCOL        *BlockIo,
>    IN  EFI_BLOCK_IO2_PROTOCOL       *BlockIo2,
>    IN  EFI_DEVICE_PATH_PROTOCOL     *DevicePath
>    )
>  {
> +  UINT32                       RemainderByMediaBlockSize;
>    EFI_STATUS                   Status;
>    EFI_BLOCK_IO_MEDIA           *Media;
>    EFI_DEVICE_PATH_PROTOCOL     *DevicePathNode;
>    EFI_GUID                     *VendorDefinedGuid;
>    EFI_GUID                     UdfDevPathGuid = EFI_UDF_DEVICE_PATH_GUID;
> @@ -246,11 +247,16 @@ PartitionInstallUdfChildHandles (
>    Media = BlockIo->Media;
>  
>    //
>    // Check if UDF logical block size is multiple of underlying device block size
>    //
> -  if ((UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize) != 0 ||
> +  DivU64x32Remainder (
> +    UDF_LOGICAL_SECTOR_SIZE,   // Dividend
> +    Media->BlockSize,          // Divisor
> +    &RemainderByMediaBlockSize // Remainder
> +    );
> +  if (RemainderByMediaBlockSize != 0 ||
>        Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE) {
>      return EFI_NOT_FOUND;
>    }
>  
>    DevicePathNode = DevicePath;
> --
> 2.14.1.3.gb7cf6e02401b
> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 5/5] MdeModulePkg/PartitionDxe: remove always false comparison

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Sunday, September 10, 2017 8:13 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 5/5] MdeModulePkg/PartitionDxe: remove always false comparison

In the expression

  (RemainderByMediaBlockSize != 0 ||
   Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE)

the second expression is only evaluated if the first expression is false.

If the first expression is false, i.e.,

  RemainderByMediaBlockSize == 0

then UDF_LOGICAL_SECTOR_SIZE is a whole multiple of "Media->BlockSize", which implies

  UDF_LOGICAL_SECTOR_SIZE >= Media->BlockSize.

Therefore whenever

  Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE

is evaluated, it is false.

The expression

  ((expression) || FALSE)

is equivalent to

  (expression).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
index c491ef25f47e..3347b48421a8 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
@@ -252,12 +252,11 @@ PartitionInstallUdfChildHandles (
   DivU64x32Remainder (
     UDF_LOGICAL_SECTOR_SIZE,   // Dividend
     Media->BlockSize,          // Divisor
     &RemainderByMediaBlockSize // Remainder
     );
-  if (RemainderByMediaBlockSize != 0 ||
-      Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE) {
+  if (RemainderByMediaBlockSize != 0) {
     return EFI_NOT_FOUND;
   }
 
   DevicePathNode = DevicePath;
   while (!IsDevicePathEnd (DevicePathNode)) {
--
2.14.1.3.gb7cf6e02401b

Re: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem()

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

BTW: How about to use "sizeof ()" instead of "sizeof"?


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Sunday, September 10, 2017 8:13 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem()

In edk2, initialization of local variables is forbidden, both for stylistic reasons and because such initialization may generate calls to compiler intrinsics.

For the following initialization in UdfRead():

  CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };

clang-3.8 generates a memset() call, when building UdfDxe for IA32, which then fails to link.

Replace the initialization with ZeroMem().

Do the same to "FilePath" in UdfOpen().

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 8b9339567f8e..e7159ff861f7 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -174,15 +174,16 @@ UdfOpen (
 {
   EFI_TPL                     OldTpl;
   EFI_STATUS                  Status;
   PRIVATE_UDF_FILE_DATA       *PrivFileData;
   PRIVATE_UDF_SIMPLE_FS_DATA  *PrivFsData;
-  CHAR16                      FilePath[UDF_PATH_LENGTH] = { 0 };
+  CHAR16                      FilePath[UDF_PATH_LENGTH];
   UDF_FILE_INFO               File;
   PRIVATE_UDF_FILE_DATA       *NewPrivFileData;
   CHAR16                      *TempFileName;
 
+  ZeroMem (FilePath, sizeof FilePath);
   OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
 
   if (This == NULL || NewHandle == NULL || FileName == NULL) {
     Status = EFI_INVALID_PARAMETER;
     goto Error_Invalid_Params;
@@ -322,14 +323,15 @@ UdfRead (
   EFI_BLOCK_IO_PROTOCOL           *BlockIo;
   EFI_DISK_IO_PROTOCOL            *DiskIo;
   UDF_FILE_INFO                   FoundFile;
   UDF_FILE_IDENTIFIER_DESCRIPTOR  *NewFileIdentifierDesc;
   VOID                            *NewFileEntryData;
-  CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
+  CHAR16                          FileName[UDF_FILENAME_LENGTH];
   UINT64                          FileSize;
   UINT64                          BufferSizeUint64;
 
+  ZeroMem (FileName, sizeof FileName);
   OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
 
   if (This == NULL || BufferSize == NULL || (*BufferSize != 0 &&
                                              Buffer == NULL)) {
     Status = EFI_INVALID_PARAMETER;
--
2.14.1.3.gb7cf6e02401b

Re: [PATCH 2/5] MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req succeeds

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Sunday, September 10, 2017 8:13 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 2/5] MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req succeeds

Ard reports that clang-3.8 correctly flags the following issue in the
ReadFile() function:

If "RecordingFlags" is INLINE_DATA, then there are three paths through the code where we mean to return success, but forget to set Status
accordingly:

(1) when "ReadFileInfo->Flags" is READ_FILE_GET_FILESIZE, or

(2) when "ReadFileInfo->Flags" is READ_FILE_ALLOCATE_AND_READ and
    AllocatePool() succeeds, or

(3) when "ReadFileInfo->Flags" is READ_FILE_SEEK_AND_READ.

Set "Status" to EFI_SUCCESS when we are done processing the INLINE_DATA request, i.e., when we reach the corresponding "break" statament under the INLINE_DATA case label.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Reported-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index a2ca65e5dfe8..0de9c71c250f 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -958,11 +958,13 @@ ReadFile (
     } else {
       ASSERT (FALSE);
       return EFI_INVALID_PARAMETER;
     }
 
+    Status = EFI_SUCCESS;
     break;
+
   case LONG_ADS_SEQUENCE:
   case SHORT_ADS_SEQUENCE:
     //
     // This FE/EFE contains a run of Allocation Descriptors. Get data + size
     // for start reading them out.
--
2.14.1.3.gb7cf6e02401b

Re: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem()

[Ni, Ruiyu]

Star,
Sizeof is an operator, not a function, like + or -. Not having () is ok.

Thanks/Ray

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, September 12, 2017 4:55 PM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-
> devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric
> <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local
> variables with ZeroMem()
> 
> Reviewed-by: Star Zeng <star.zeng@intel.com>
> 
> BTW: How about to use "sizeof ()" instead of "sizeof"?
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Sunday, September 10, 2017 8:13 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric
> <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local
> variables with ZeroMem()
> 
> In edk2, initialization of local variables is forbidden, both for stylistic reasons
> and because such initialization may generate calls to compiler intrinsics.
> 
> For the following initialization in UdfRead():
> 
>   CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
> 
> clang-3.8 generates a memset() call, when building UdfDxe for IA32, which
> then fails to link.
> 
> Replace the initialization with ZeroMem().
> 
> Do the same to "FilePath" in UdfOpen().
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Paulo Alcantara <pcacjr@zytor.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> index 8b9339567f8e..e7159ff861f7 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -174,15 +174,16 @@ UdfOpen (
>  {
>    EFI_TPL                     OldTpl;
>    EFI_STATUS                  Status;
>    PRIVATE_UDF_FILE_DATA       *PrivFileData;
>    PRIVATE_UDF_SIMPLE_FS_DATA  *PrivFsData;
> -  CHAR16                      FilePath[UDF_PATH_LENGTH] = { 0 };
> +  CHAR16                      FilePath[UDF_PATH_LENGTH];
>    UDF_FILE_INFO               File;
>    PRIVATE_UDF_FILE_DATA       *NewPrivFileData;
>    CHAR16                      *TempFileName;
> 
> +  ZeroMem (FilePath, sizeof FilePath);
>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> 
>    if (This == NULL || NewHandle == NULL || FileName == NULL) {
>      Status = EFI_INVALID_PARAMETER;
>      goto Error_Invalid_Params;
> @@ -322,14 +323,15 @@ UdfRead (
>    EFI_BLOCK_IO_PROTOCOL           *BlockIo;
>    EFI_DISK_IO_PROTOCOL            *DiskIo;
>    UDF_FILE_INFO                   FoundFile;
>    UDF_FILE_IDENTIFIER_DESCRIPTOR  *NewFileIdentifierDesc;
>    VOID                            *NewFileEntryData;
> -  CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
> +  CHAR16                          FileName[UDF_FILENAME_LENGTH];
>    UINT64                          FileSize;
>    UINT64                          BufferSizeUint64;
> 
> +  ZeroMem (FileName, sizeof FileName);
>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> 
>    if (This == NULL || BufferSize == NULL || (*BufferSize != 0 &&
>                                               Buffer == NULL)) {
>      Status = EFI_INVALID_PARAMETER;
> --
> 2.14.1.3.gb7cf6e02401b
> 

Re: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem()

[Laszlo Ersek]

On 09/12/17 10:55, Zeng, Star wrote:
> Reviewed-by: Star Zeng <star.zeng@intel.com>
> 
> BTW: How about to use "sizeof ()" instead of "sizeof"?

"sizeof" is a unary operator. The parentheses are mandatory when sizeof
is used with a type name, but when the operand of sizeof is an
expression, the parentheses frequently make the wrong impression.

Consider for example

  UINTN          Var1, Var2, Var3;
  STRUCTURE_TYPE VarStruct;

  Var1 = 5;
  Var2 = sizeof Var3 + Var1;        // [1]

Most people dislike the assignment to Var2, and will rewrite it as:

  Var2 = sizeof (Var3) + Var1;      // [2]

However, this is totally irrelevant, and does not reflect the binding
strengths between the "sizeof" and "+" operators. In order to reflect
that relationship correctly, the following parentheses should be added:

  Var2 = (sizeof Var3) + Var1;      // [3]

None of [2] or [3] make any difference relative to [1], of course, in
behavior.

I just dislike [2] because it *suggests* that putting Var3 in parens
"protects" Var1 from falling under "sizeof".

That's not the case. "sizeof" is not a function, it is a unary operator
like "*", "&", "!", etc.

If we want to add parens for emphasizing the actual operator binding,
then [3] is the way to go. For example, if "Var3" was a pointer, you
wouldn't write

  *(Var3) + Var1

in order to emphasize that "*" takes precedence over "+";  you would write

  (*Var3) + Var1


TL;DR: "sizeof" is a unary, prefix operator, not a function designator.

... In order not to delay this any longer, I will add the parens before
I push.

But, we should all know that those parens don't mean what most people
think they mean :)

Laszlo


> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Sunday, September 10, 2017 8:13 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem()
> 
> In edk2, initialization of local variables is forbidden, both for stylistic reasons and because such initialization may generate calls to compiler intrinsics.
> 
> For the following initialization in UdfRead():
> 
>   CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
> 
> clang-3.8 generates a memset() call, when building UdfDxe for IA32, which then fails to link.
> 
> Replace the initialization with ZeroMem().
> 
> Do the same to "FilePath" in UdfOpen().
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Paulo Alcantara <pcacjr@zytor.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> index 8b9339567f8e..e7159ff861f7 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -174,15 +174,16 @@ UdfOpen (
>  {
>    EFI_TPL                     OldTpl;
>    EFI_STATUS                  Status;
>    PRIVATE_UDF_FILE_DATA       *PrivFileData;
>    PRIVATE_UDF_SIMPLE_FS_DATA  *PrivFsData;
> -  CHAR16                      FilePath[UDF_PATH_LENGTH] = { 0 };
> +  CHAR16                      FilePath[UDF_PATH_LENGTH];
>    UDF_FILE_INFO               File;
>    PRIVATE_UDF_FILE_DATA       *NewPrivFileData;
>    CHAR16                      *TempFileName;
>  
> +  ZeroMem (FilePath, sizeof FilePath);
>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>  
>    if (This == NULL || NewHandle == NULL || FileName == NULL) {
>      Status = EFI_INVALID_PARAMETER;
>      goto Error_Invalid_Params;
> @@ -322,14 +323,15 @@ UdfRead (
>    EFI_BLOCK_IO_PROTOCOL           *BlockIo;
>    EFI_DISK_IO_PROTOCOL            *DiskIo;
>    UDF_FILE_INFO                   FoundFile;
>    UDF_FILE_IDENTIFIER_DESCRIPTOR  *NewFileIdentifierDesc;
>    VOID                            *NewFileEntryData;
> -  CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
> +  CHAR16                          FileName[UDF_FILENAME_LENGTH];
>    UINT64                          FileSize;
>    UINT64                          BufferSizeUint64;
>  
> +  ZeroMem (FileName, sizeof FileName);
>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>  
>    if (This == NULL || BufferSize == NULL || (*BufferSize != 0 &&
>                                               Buffer == NULL)) {
>      Status = EFI_INVALID_PARAMETER;
> --
> 2.14.1.3.gb7cf6e02401b
> 
> 

Re: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem()

[Laszlo Ersek]

On 09/12/17 11:38, Ni, Ruiyu wrote:
> Star,
> Sizeof is an operator, not a function, like + or -. Not having () is ok.

Ugh, just seeing this now :) So what should I do now?

If Star agrees, I would prefer *not* to add the parens. If Star insists,
I can add them.

Thanks
Laszlo

>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Tuesday, September 12, 2017 4:55 PM
>> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-
>> devel@lists.01.org>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric
>> <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu
>> <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: RE: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local
>> variables with ZeroMem()
>>
>> Reviewed-by: Star Zeng <star.zeng@intel.com>
>>
>> BTW: How about to use "sizeof ()" instead of "sizeof"?
>>
>>
>> Thanks,
>> Star
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Sunday, September 10, 2017 8:13 AM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric
>> <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu
>> <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local
>> variables with ZeroMem()
>>
>> In edk2, initialization of local variables is forbidden, both for stylistic reasons
>> and because such initialization may generate calls to compiler intrinsics.
>>
>> For the following initialization in UdfRead():
>>
>>   CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
>>
>> clang-3.8 generates a memset() call, when building UdfDxe for IA32, which
>> then fails to link.
>>
>> Replace the initialization with ZeroMem().
>>
>> Do the same to "FilePath" in UdfOpen().
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Paulo Alcantara <pcacjr@zytor.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> index 8b9339567f8e..e7159ff861f7 100644
>> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> @@ -174,15 +174,16 @@ UdfOpen (
>>  {
>>    EFI_TPL                     OldTpl;
>>    EFI_STATUS                  Status;
>>    PRIVATE_UDF_FILE_DATA       *PrivFileData;
>>    PRIVATE_UDF_SIMPLE_FS_DATA  *PrivFsData;
>> -  CHAR16                      FilePath[UDF_PATH_LENGTH] = { 0 };
>> +  CHAR16                      FilePath[UDF_PATH_LENGTH];
>>    UDF_FILE_INFO               File;
>>    PRIVATE_UDF_FILE_DATA       *NewPrivFileData;
>>    CHAR16                      *TempFileName;
>>
>> +  ZeroMem (FilePath, sizeof FilePath);
>>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>>
>>    if (This == NULL || NewHandle == NULL || FileName == NULL) {
>>      Status = EFI_INVALID_PARAMETER;
>>      goto Error_Invalid_Params;
>> @@ -322,14 +323,15 @@ UdfRead (
>>    EFI_BLOCK_IO_PROTOCOL           *BlockIo;
>>    EFI_DISK_IO_PROTOCOL            *DiskIo;
>>    UDF_FILE_INFO                   FoundFile;
>>    UDF_FILE_IDENTIFIER_DESCRIPTOR  *NewFileIdentifierDesc;
>>    VOID                            *NewFileEntryData;
>> -  CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
>> +  CHAR16                          FileName[UDF_FILENAME_LENGTH];
>>    UINT64                          FileSize;
>>    UINT64                          BufferSizeUint64;
>>
>> +  ZeroMem (FileName, sizeof FileName);
>>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>>
>>    if (This == NULL || BufferSize == NULL || (*BufferSize != 0 &&
>>                                               Buffer == NULL)) {
>>      Status = EFI_INVALID_PARAMETER;
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
> 

Re: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem()

[Zeng, Star]

I know sizeof works same with sizeof () here, I have no preference, that's why I gave Reviewed-by before the comment, I just see most of the code are using sizeof (), I am not sure whether there is a code standard for it somewhere, I guess no.

Anyway, you can make the decision when pushing. :)


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, September 12, 2017 5:57 PM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>
Subject: Re: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem()

On 09/12/17 11:38, Ni, Ruiyu wrote:
> Star,
> Sizeof is an operator, not a function, like + or -. Not having () is ok.

Ugh, just seeing this now :) So what should I do now?

If Star agrees, I would prefer *not* to add the parens. If Star insists, I can add them.

Thanks
Laszlo

>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Tuesday, September 12, 2017 4:55 PM
>> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2- 
>> devel@lists.01.org>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric 
>> <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu 
>> <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: RE: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of 
>> local variables with ZeroMem()
>>
>> Reviewed-by: Star Zeng <star.zeng@intel.com>
>>
>> BTW: How about to use "sizeof ()" instead of "sizeof"?
>>
>>
>> Thanks,
>> Star
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Sunday, September 10, 2017 8:13 AM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric 
>> <eric.dong@intel.com>; Paulo Alcantara <pcacjr@zytor.com>; Ni, Ruiyu 
>> <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local 
>> variables with ZeroMem()
>>
>> In edk2, initialization of local variables is forbidden, both for 
>> stylistic reasons and because such initialization may generate calls to compiler intrinsics.
>>
>> For the following initialization in UdfRead():
>>
>>   CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
>>
>> clang-3.8 generates a memset() call, when building UdfDxe for IA32, 
>> which then fails to link.
>>
>> Replace the initialization with ZeroMem().
>>
>> Do the same to "FilePath" in UdfOpen().
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Paulo Alcantara <pcacjr@zytor.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> index 8b9339567f8e..e7159ff861f7 100644
>> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> @@ -174,15 +174,16 @@ UdfOpen (
>>  {
>>    EFI_TPL                     OldTpl;
>>    EFI_STATUS                  Status;
>>    PRIVATE_UDF_FILE_DATA       *PrivFileData;
>>    PRIVATE_UDF_SIMPLE_FS_DATA  *PrivFsData;
>> -  CHAR16                      FilePath[UDF_PATH_LENGTH] = { 0 };
>> +  CHAR16                      FilePath[UDF_PATH_LENGTH];
>>    UDF_FILE_INFO               File;
>>    PRIVATE_UDF_FILE_DATA       *NewPrivFileData;
>>    CHAR16                      *TempFileName;
>>
>> +  ZeroMem (FilePath, sizeof FilePath);
>>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>>
>>    if (This == NULL || NewHandle == NULL || FileName == NULL) {
>>      Status = EFI_INVALID_PARAMETER;
>>      goto Error_Invalid_Params;
>> @@ -322,14 +323,15 @@ UdfRead (
>>    EFI_BLOCK_IO_PROTOCOL           *BlockIo;
>>    EFI_DISK_IO_PROTOCOL            *DiskIo;
>>    UDF_FILE_INFO                   FoundFile;
>>    UDF_FILE_IDENTIFIER_DESCRIPTOR  *NewFileIdentifierDesc;
>>    VOID                            *NewFileEntryData;
>> -  CHAR16                          FileName[UDF_FILENAME_LENGTH] = { 0 };
>> +  CHAR16                          FileName[UDF_FILENAME_LENGTH];
>>    UINT64                          FileSize;
>>    UINT64                          BufferSizeUint64;
>>
>> +  ZeroMem (FileName, sizeof FileName);
>>    OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>>
>>    if (This == NULL || BufferSize == NULL || (*BufferSize != 0 &&
>>                                               Buffer == NULL)) {
>>      Status = EFI_INVALID_PARAMETER;
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
> 

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Laszlo Ersek]

On 09/10/17 02:12, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: udf_fixes_cleanups
> 
> Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for
> IA32 and X64 with clang-3.8, after the UDF introduction.
> 
> Patches #1 and #5 are cleanups that I felt fit before patch #2 and after
> patch #4, respectively.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Paulo Alcantara <pcacjr@zytor.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (5):
>   MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA
>     req
>   MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req
>     succeeds
>   MdeModulePkg/UdfDxe: replace zero-init of local variables with
>     ZeroMem()
>   MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators
>   MdeModulePkg/PartitionDxe: remove always false comparison
> 
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c            | 9 +++++++--
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c                 | 6 ++++--
>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 

Thanks all for the feedback, pushed as commit range
c05cae55ebd8..b4e5807d2492.

(I didn't change the sizeof / sizeof() stuff -- for one, I didn't want
to touch the code on such an urgent push, relative to the posted and
tested version.)

Thanks
Laszlo

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Ard Biesheuvel]

On 12 September 2017 at 03:14, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/10/17 02:12, Laszlo Ersek wrote:
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: udf_fixes_cleanups
>>
>> Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for
>> IA32 and X64 with clang-3.8, after the UDF introduction.
>>
>> Patches #1 and #5 are cleanups that I felt fit before patch #2 and after
>> patch #4, respectively.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Paulo Alcantara <pcacjr@zytor.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (5):
>>   MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA
>>     req
>>   MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req
>>     succeeds
>>   MdeModulePkg/UdfDxe: replace zero-init of local variables with
>>     ZeroMem()
>>   MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators
>>   MdeModulePkg/PartitionDxe: remove always false comparison
>>
>>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c            | 9 +++++++--
>>  MdeModulePkg/Universal/Disk/UdfDxe/File.c                 | 6 ++++--
>>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++
>>  3 files changed, 16 insertions(+), 4 deletions(-)
>>
>
> Thanks all for the feedback, pushed as commit range
> c05cae55ebd8..b4e5807d2492.
>
> (I didn't change the sizeof / sizeof() stuff -- for one, I didn't want
> to touch the code on such an urgent push, relative to the posted and
> tested version.)
>

Rather unexpectedly, the build is still broken on my CI system

This time, it is GCC 4.8 that complains about uninitialized variables,
most likely false positives again (apologies for the letter soup)

<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:
In function 'ReadFile':
<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:876:27:
error: 'Status' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
   EFI_STATUS              Status;
                           ^
"/home/buildslave/srv/toolchain/arm-tc-14.04/bin/arm-linux-gnueabihf-gcc"
-mthumb -mcpu=cortex-a15
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/ArmVirtPkg/Include>
-g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror
-Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian
-mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections
-fdata-sections -fomit-frame-pointer -Wno-address -mthumb
-mfloat-abi=soft -fno-pic -fno-pie -fstack-protector
-mword-relocations -Wno-unused-but-set-variable -DMDEPKG_NDEBUG
-DDISABLE_NEW_DEPRECATED_INTERFACES -c -o
<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/Build/ArmVirtQemu-ARM/RELEASE_GCC48/ARM/OvmfPkg/VirtioNetDxe/VirtioNet/OUTPUT/./SnpSharedHelpers.obj>
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg/VirtioNetDxe>
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/Build/ArmVirtQemu-ARM/RELEASE_GCC48/ARM/OvmfPkg/VirtioNetDxe/VirtioNet/DEBUG>
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdePkg>
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdePkg/Include>
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdePkg/Include/Arm>
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg>
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg/Include>
<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c>
<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:887:27:
error: 'BytesLeft' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
   UINT64                  BytesLeft;
                           ^
cc1: all warnings being treated as errors

Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Laszlo Ersek]

On 09/12/17 17:38, Ard Biesheuvel wrote:
> On 12 September 2017 at 03:14, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 09/10/17 02:12, Laszlo Ersek wrote:
>>> Repo:   https://github.com/lersek/edk2.git
>>> Branch: udf_fixes_cleanups
>>>
>>> Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for
>>> IA32 and X64 with clang-3.8, after the UDF introduction.
>>>
>>> Patches #1 and #5 are cleanups that I felt fit before patch #2 and after
>>> patch #4, respectively.
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Paulo Alcantara <pcacjr@zytor.com>
>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>>
>>> Thanks
>>> Laszlo
>>>
>>> Laszlo Ersek (5):
>>>   MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA
>>>     req
>>>   MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req
>>>     succeeds
>>>   MdeModulePkg/UdfDxe: replace zero-init of local variables with
>>>     ZeroMem()
>>>   MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators
>>>   MdeModulePkg/PartitionDxe: remove always false comparison
>>>
>>>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c            | 9 +++++++--
>>>  MdeModulePkg/Universal/Disk/UdfDxe/File.c                 | 6 ++++--
>>>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++
>>>  3 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>
>> Thanks all for the feedback, pushed as commit range
>> c05cae55ebd8..b4e5807d2492.
>>
>> (I didn't change the sizeof / sizeof() stuff -- for one, I didn't want
>> to touch the code on such an urgent push, relative to the posted and
>> tested version.)
>>
> 
> Rather unexpectedly, the build is still broken on my CI system
> 
> This time, it is GCC 4.8 that complains about uninitialized variables,
> most likely false positives again (apologies for the letter soup)
> 
> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:
> In function 'ReadFile':
> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:876:27:
> error: 'Status' may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
>    EFI_STATUS              Status;
>                            ^

This is actually a bug in the code.

In order to explain that, I have to elaborate.

The UDF standard from the OSTA is introduced like this:

    The OSTA Universal Disk Format (UDF ® ) specification defines a
    subset of the standard ECMA 167 3 rd edition. The primary goal of
    the OSTA UDF is to maximize data interchange and minimize the cost
    and complexity of implementing ECMA 167.

Which (in retrospect...) means that we have to download ECMA 167 at once:

https://www.ecma-international.org/publications/standards/Ecma-167.htm

OK, so let's look at the code.

First, the ReadFile() function is very hard to read. It is long (~300
lines), but what is actually problematic about it is that it uses "goto"
statements for control flow that is *not* related to error handling.
This makes it painful to track, and it happens to violate the edk2
coding style spec as well:

https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/57_c_programming.html#57-c-programming

    5.7.3.8 Goto Statements should not be used (in general)

    In almost all cases, it is possible to write the code so that a goto
    is not needed. If a goto is used, be ready to defend it during
    review.

    It is common to use goto for error handling and thus, exiting a
    routine in an error case is the only legal use of a goto. A goto
    allows the error exit code to be contained in one place in the
    routine. This one place reduces software life cycle maintenance
    issues, as there can be one copy of error cleanup code per routine.

   [...]

Second, in this case it actually pays off to read the function top-down.

- The first switch statement (on "ReadFileInfo->Flags") does not set
Status, but it also doesn't read it or jump to a read site.

- The second switch statement has four case labels (RecordingFlags):

  - INLINE_DATA: it sets Status to EFI_SUCCESS
    (thanks to my recent patch)

  - LONG_ADS_SEQUENCE, SHORT_ADS_SEQUENCE: Status is set very
    soon, when we unconditionally call GetAllocationDescriptor().

  - EXTENDED_ADS_SEQUENCE: we ASSERT(FALSE), but also set Status to
    EFI_UNSUPPORTED

Then, assuming we didn't jump to any of the error labels
(Error_Read_Disk_Blk, Error_Alloc_Buffer_To_Next_Ad, Error_Get_Aed), we
proceed to the Done label, and return Status.

(BTW, I checked all the paths to those error labels, and those *are* fine.)

So how is it possible that we reach Done, and return Status, without
having set Status? It is possible by taking *none* of the branches in
the second switch statement (RecordingFlags). The switch statement does
not have a default label, which also happens to break the coding style:

  5.7.3.7 switch Statements

  [...]

  Always include the default case. It should always end with a break
  statement, even if the break is the last thing before the closing
  brace.

OK, so how is it possible that none of the case labels match?
"RecordingFlags", i.e. the controlling expression of the switch
statement, is set like this:

  RecordingFlags = GET_FE_RECORDING_FLAGS (FileEntryData);

which in turn is #defined as

typedef enum {
  SHORT_ADS_SEQUENCE,
  LONG_ADS_SEQUENCE,
  EXTENDED_ADS_SEQUENCE,
  INLINE_DATA
} UDF_FE_RECORDING_FLAGS;

#define GET_FE_RECORDING_FLAGS(_Fe) \
  ((UDF_FE_RECORDING_FLAGS)((UDF_ICB_TAG *)( \
                  (UINT8 *)(_Fe) + \
                  sizeof (UDF_DESCRIPTOR_TAG)))->Flags & 0x07)

So basically we take a bitmask which may have values 0..7 decimal
inclusive (bits 2:0), and then compare it against values 0, 1, 2, 3
only. Because this value comes from an on-disk data structure, i.e. we
don't set it ourselves in the driver, this is an actual bug. A default
label should be added to reject the unsupported cases.

Now, the UDF spec documents this field under

2.3.5 ICB Tag
2.3.5.4 Uint16 Flags
  Bits 0-2: These bits specify the type of allocation descriptors used.
  Refer to section 2.3.10 on Allocation Descriptors for the guidelines
  on choosing which type of allocation descriptor to use.

So we go to 2.3.10:

2.3.10 Allocation Descriptors
[...]

and we find that from the 4 constants listed in the code, the UDF spec
names only two, "Short Allocation Descriptor" (SHORT_ADS_SEQUENCE) and
"Long Allocation Descriptor" (LONG_ADS_SEQUENCE), but the spec doesn't
even provide their numerical values!

This is where ECMA-167 becomes relevant. We have:

14.6 ICB Tag
14.6.8 Flags (RBP 18)
Bit  Interpretation
0-2  Shall be interpreted as a 3-bit unsigned binary number as follows.
     The value 0 shall mean that Short Allocation Descriptors
     (4/14.14.1) are used. The value 1 shall mean that Long Allocation
     Descriptors (4/14.14.2) are used. The value 2 shall mean that
     Extended Allocation Descriptors (4/14.14.3) are used. The value 3
     shall mean that the file shall be treated as though it had exactly
     one allocation descriptor describing an extent which starts with
     the first byte of the Allocation Descriptors field and has a
     length, in bytes, recorded in the Length of Allocation Descriptors
     field. The values of 4-7 are reserved for future standardisation.

The enumeration constants in UDF_FE_RECORDING_FLAGS cover the specified
cases, but the reserved values are not checked against in the 2nd switch
statement, under default case label.

This also proves that setting Status at the top of the function, to
EFI_SUCCESS namely, would have been wrong in commit 131fd40ffc34.

> "/home/buildslave/srv/toolchain/arm-tc-14.04/bin/arm-linux-gnueabihf-gcc"
> -mthumb -mcpu=cortex-a15
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/ArmVirtPkg/Include>
> -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror
> -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian
> -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections
> -fdata-sections -fomit-frame-pointer -Wno-address -mthumb
> -mfloat-abi=soft -fno-pic -fno-pie -fstack-protector
> -mword-relocations -Wno-unused-but-set-variable -DMDEPKG_NDEBUG
> -DDISABLE_NEW_DEPRECATED_INTERFACES -c -o
> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/Build/ArmVirtQemu-ARM/RELEASE_GCC48/ARM/OvmfPkg/VirtioNetDxe/VirtioNet/OUTPUT/./SnpSharedHelpers.obj>
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg/VirtioNetDxe>
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/Build/ArmVirtQemu-ARM/RELEASE_GCC48/ARM/OvmfPkg/VirtioNetDxe/VirtioNet/DEBUG>
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdePkg>
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdePkg/Include>
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdePkg/Include/Arm>
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg>
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg/Include>
> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c>
> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:887:27:
> error: 'BytesLeft' may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
>    UINT64                  BytesLeft;
>                            ^

This is a false positive indeed. There are three reads of "BytesLeft",
and they are all under:

  LONG_ADS_SEQUENCE / SHORT_ADS_SEQUENCE
    READ_FILE_SEEK_AND_READ

However, BytesLeft is also assigned near the top of the function, under
READ_FILE_SEEK_AND_READ.

(Of course, this analysis is complicated by the fact that we have a
non-error handling label, namely "Skip_File_Seek", just above the first
read location of "BytesLeft". So we have to verify that nothing jumps
there without having set "BytesLeft". Goto statements that aren't used
for error handling / end-of-function cleanup are horrible.)

I'm going to submit two patches for these issues, but that's officially
the end of the resources I can allocate for this driver now. All further
error reports, even build errors, must go into the TianoCore Bugzilla;
no exceptions.

Laszlo