* [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups @ 2017-09-10 0:12 Laszlo Ersek 2017-09-10 0:13 ` [PATCH 1/5] MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA req Laszlo Ersek ` (8 more replies) 0 siblings, 9 replies; 34+ messages in thread From: Laszlo Ersek @ 2017-09-10 0:12 UTC (permalink / raw) To: edk2-devel-01 Cc: Ard Biesheuvel, Eric Dong, Paulo Alcantara, Ruiyu Ni, Star Zeng 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/5] MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA req 2017-09-10 0:12 [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups Laszlo Ersek @ 2017-09-10 0:13 ` Laszlo Ersek 2017-09-10 0:13 ` [PATCH 2/5] MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req succeeds Laszlo Ersek ` (7 subsequent siblings) 8 siblings, 0 replies; 34+ messages in thread From: Laszlo Ersek @ 2017-09-10 0:13 UTC (permalink / raw) To: edk2-devel-01 Cc: Ard Biesheuvel, Eric Dong, Paulo Alcantara, Ruiyu Ni, Star Zeng 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 2/5] MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req succeeds 2017-09-10 0:12 [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups Laszlo Ersek 2017-09-10 0:13 ` [PATCH 1/5] MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA req Laszlo Ersek @ 2017-09-10 0:13 ` Laszlo Ersek 2017-09-12 9:06 ` Zeng, Star 2017-09-10 0:13 ` [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem() Laszlo Ersek ` (6 subsequent siblings) 8 siblings, 1 reply; 34+ messages in thread From: Laszlo Ersek @ 2017-09-10 0:13 UTC (permalink / raw) To: edk2-devel-01 Cc: Ard Biesheuvel, Eric Dong, Paulo Alcantara, Ruiyu Ni, Star Zeng 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 2/5] MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req succeeds 2017-09-10 0:13 ` [PATCH 2/5] MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req succeeds Laszlo Ersek @ 2017-09-12 9:06 ` Zeng, Star 0 siblings, 0 replies; 34+ messages in thread From: Zeng, Star @ 2017-09-12 9:06 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Cc: Ard Biesheuvel, Dong, Eric, Paulo Alcantara, Ni, Ruiyu, 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem() 2017-09-10 0:12 [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups Laszlo Ersek 2017-09-10 0:13 ` [PATCH 1/5] MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA req Laszlo Ersek 2017-09-10 0:13 ` [PATCH 2/5] MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req succeeds Laszlo Ersek @ 2017-09-10 0:13 ` Laszlo Ersek 2017-09-12 8:55 ` Zeng, Star 2017-09-10 0:13 ` [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators Laszlo Ersek ` (5 subsequent siblings) 8 siblings, 1 reply; 34+ messages in thread From: Laszlo Ersek @ 2017-09-10 0:13 UTC (permalink / raw) To: edk2-devel-01 Cc: Ard Biesheuvel, Eric Dong, Paulo Alcantara, Ruiyu Ni, Star Zeng 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem() 2017-09-10 0:13 ` [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem() Laszlo Ersek @ 2017-09-12 8:55 ` Zeng, Star 2017-09-12 9:38 ` Ni, Ruiyu 2017-09-12 9:55 ` Laszlo Ersek 0 siblings, 2 replies; 34+ messages in thread From: Zeng, Star @ 2017-09-12 8:55 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Cc: Ard Biesheuvel, Dong, Eric, Paulo Alcantara, Ni, Ruiyu, 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem() 2017-09-12 8:55 ` Zeng, Star @ 2017-09-12 9:38 ` Ni, Ruiyu 2017-09-12 9:57 ` Laszlo Ersek 2017-09-12 9:55 ` Laszlo Ersek 1 sibling, 1 reply; 34+ messages in thread From: Ni, Ruiyu @ 2017-09-12 9:38 UTC (permalink / raw) To: Zeng, Star, Laszlo Ersek, edk2-devel-01 Cc: Ard Biesheuvel, Dong, Eric, Paulo Alcantara 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 > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem() 2017-09-12 9:38 ` Ni, Ruiyu @ 2017-09-12 9:57 ` Laszlo Ersek 2017-09-12 10:02 ` Zeng, Star 0 siblings, 1 reply; 34+ messages in thread From: Laszlo Ersek @ 2017-09-12 9:57 UTC (permalink / raw) To: Ni, Ruiyu, Zeng, Star, edk2-devel-01 Cc: Ard Biesheuvel, Dong, Eric, Paulo Alcantara 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 >> > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem() 2017-09-12 9:57 ` Laszlo Ersek @ 2017-09-12 10:02 ` Zeng, Star 0 siblings, 0 replies; 34+ messages in thread From: Zeng, Star @ 2017-09-12 10:02 UTC (permalink / raw) To: Laszlo Ersek, Ni, Ruiyu, edk2-devel-01 Cc: Ard Biesheuvel, Dong, Eric, Paulo Alcantara, 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 >> > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem() 2017-09-12 8:55 ` Zeng, Star 2017-09-12 9:38 ` Ni, Ruiyu @ 2017-09-12 9:55 ` Laszlo Ersek 1 sibling, 0 replies; 34+ messages in thread From: Laszlo Ersek @ 2017-09-12 9:55 UTC (permalink / raw) To: Zeng, Star, edk2-devel-01 Cc: Ard Biesheuvel, Dong, Eric, Paulo Alcantara, Ni, Ruiyu 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 > > ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators 2017-09-10 0:12 [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups Laszlo Ersek ` (2 preceding siblings ...) 2017-09-10 0:13 ` [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem() Laszlo Ersek @ 2017-09-10 0:13 ` Laszlo Ersek 2017-09-12 5:41 ` Bi, Dandan 2017-09-10 0:13 ` [PATCH 5/5] MdeModulePkg/PartitionDxe: remove always false comparison Laszlo Ersek ` (4 subsequent siblings) 8 siblings, 1 reply; 34+ messages in thread From: Laszlo Ersek @ 2017-09-10 0:13 UTC (permalink / raw) To: edk2-devel-01 Cc: Ard Biesheuvel, Eric Dong, Paulo Alcantara, Ruiyu Ni, Star Zeng 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators 2017-09-10 0:13 ` [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators Laszlo Ersek @ 2017-09-12 5:41 ` Bi, Dandan 2017-09-12 7:58 ` Laszlo Ersek 0 siblings, 1 reply; 34+ messages in thread From: Bi, Dandan @ 2017-09-12 5:41 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Cc: Ni, Ruiyu, Dong, Eric, Zeng, Star, Ard Biesheuvel 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators 2017-09-12 5:41 ` Bi, Dandan @ 2017-09-12 7:58 ` Laszlo Ersek 2017-09-12 8:28 ` Ni, Ruiyu 2017-09-12 8:46 ` Zeng, Star 0 siblings, 2 replies; 34+ messages in thread From: Laszlo Ersek @ 2017-09-12 7:58 UTC (permalink / raw) To: Bi, Dandan, edk2-devel-01 Cc: Ni, Ruiyu, Dong, Eric, Zeng, Star, Ard Biesheuvel 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 > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators 2017-09-12 7:58 ` Laszlo Ersek @ 2017-09-12 8:28 ` Ni, Ruiyu 2017-09-12 8:46 ` Zeng, Star 1 sibling, 0 replies; 34+ messages in thread From: Ni, Ruiyu @ 2017-09-12 8:28 UTC (permalink / raw) To: Laszlo Ersek, Bi, Dandan, edk2-devel-01 Cc: Dong, Eric, Zeng, Star, Ard Biesheuvel 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 > > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators 2017-09-12 7:58 ` Laszlo Ersek 2017-09-12 8:28 ` Ni, Ruiyu @ 2017-09-12 8:46 ` Zeng, Star 1 sibling, 0 replies; 34+ messages in thread From: Zeng, Star @ 2017-09-12 8:46 UTC (permalink / raw) To: Laszlo Ersek, Bi, Dandan, edk2-devel-01 Cc: Ni, Ruiyu, Dong, Eric, Ard Biesheuvel, 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 > ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 5/5] MdeModulePkg/PartitionDxe: remove always false comparison 2017-09-10 0:12 [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups Laszlo Ersek ` (3 preceding siblings ...) 2017-09-10 0:13 ` [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators Laszlo Ersek @ 2017-09-10 0:13 ` Laszlo Ersek 2017-09-12 8:50 ` Zeng, Star 2017-09-10 4:24 ` [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups Shi, Steven ` (3 subsequent siblings) 8 siblings, 1 reply; 34+ messages in thread From: Laszlo Ersek @ 2017-09-10 0:13 UTC (permalink / raw) To: edk2-devel-01 Cc: Ard Biesheuvel, Eric Dong, Paulo Alcantara, Ruiyu Ni, Star Zeng 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 5/5] MdeModulePkg/PartitionDxe: remove always false comparison 2017-09-10 0:13 ` [PATCH 5/5] MdeModulePkg/PartitionDxe: remove always false comparison Laszlo Ersek @ 2017-09-12 8:50 ` Zeng, Star 0 siblings, 0 replies; 34+ messages in thread From: Zeng, Star @ 2017-09-12 8:50 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Cc: Ard Biesheuvel, Dong, Eric, Paulo Alcantara, Ni, Ruiyu, 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-10 0:12 [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups Laszlo Ersek ` (4 preceding siblings ...) 2017-09-10 0:13 ` [PATCH 5/5] MdeModulePkg/PartitionDxe: remove always false comparison Laszlo Ersek @ 2017-09-10 4:24 ` Shi, Steven 2017-09-10 8:38 ` Laszlo Ersek 2017-09-10 15:05 ` Paulo Alcantara ` (2 subsequent siblings) 8 siblings, 1 reply; 34+ messages in thread From: Shi, Steven @ 2017-09-10 4:24 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Cc: Ni, Ruiyu, Dong, Eric, Zeng, Star, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-10 4:24 ` [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups Shi, Steven @ 2017-09-10 8:38 ` Laszlo Ersek 2017-09-10 13:52 ` Laszlo Ersek 0 siblings, 1 reply; 34+ messages in thread From: Laszlo Ersek @ 2017-09-10 8:38 UTC (permalink / raw) To: Shi, Steven, edk2-devel-01 Cc: Ni, Ruiyu, Dong, Eric, Zeng, Star, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-10 8:38 ` Laszlo Ersek @ 2017-09-10 13:52 ` Laszlo Ersek 2017-09-10 14:27 ` Shi, Steven 0 siblings, 1 reply; 34+ messages in thread From: Laszlo Ersek @ 2017-09-10 13:52 UTC (permalink / raw) To: Shi, Steven, edk2-devel-01 Cc: Ni, Ruiyu, Dong, Eric, Zeng, Star, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-10 13:52 ` Laszlo Ersek @ 2017-09-10 14:27 ` Shi, Steven 2017-09-10 15:51 ` Paulo Alcantara 0 siblings, 1 reply; 34+ messages in thread From: Shi, Steven @ 2017-09-10 14:27 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Cc: Ni, Ruiyu, Dong, Eric, Zeng, Star, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-10 14:27 ` Shi, Steven @ 2017-09-10 15:51 ` Paulo Alcantara 2017-09-11 6:58 ` Laszlo Ersek 2017-09-11 13:07 ` Shi, Steven 0 siblings, 2 replies; 34+ messages in thread From: Paulo Alcantara @ 2017-09-10 15:51 UTC (permalink / raw) To: Shi, Steven, Laszlo Ersek, edk2-devel-01 Cc: Ni, Ruiyu, Dong, Eric, Zeng, Star, Ard Biesheuvel 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 > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-10 15:51 ` Paulo Alcantara @ 2017-09-11 6:58 ` Laszlo Ersek 2017-09-11 13:55 ` Paulo Alcantara 2017-09-11 13:07 ` Shi, Steven 1 sibling, 1 reply; 34+ messages in thread From: Laszlo Ersek @ 2017-09-11 6:58 UTC (permalink / raw) To: Paulo Alcantara, Shi, Steven, edk2-devel-01 Cc: Ni, Ruiyu, Dong, Eric, Zeng, Star, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-11 6:58 ` Laszlo Ersek @ 2017-09-11 13:55 ` Paulo Alcantara 0 siblings, 0 replies; 34+ messages in thread From: Paulo Alcantara @ 2017-09-11 13:55 UTC (permalink / raw) To: Laszlo Ersek, Shi, Steven, edk2-devel-01 Cc: Ni, Ruiyu, Dong, Eric, Zeng, Star, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-10 15:51 ` Paulo Alcantara 2017-09-11 6:58 ` Laszlo Ersek @ 2017-09-11 13:07 ` Shi, Steven 2017-09-11 13:26 ` Ni, Ruiyu ` (2 more replies) 1 sibling, 3 replies; 34+ messages in thread From: Shi, Steven @ 2017-09-11 13:07 UTC (permalink / raw) To: Paulo Alcantara, Laszlo Ersek, edk2-devel-01 Cc: Ni, Ruiyu, Dong, Eric, Zeng, Star, Ard Biesheuvel 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 > > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-11 13:07 ` Shi, Steven @ 2017-09-11 13:26 ` Ni, Ruiyu 2017-09-11 13:26 ` Laszlo Ersek 2017-09-11 13:52 ` Paulo Alcantara 2 siblings, 0 replies; 34+ messages in thread From: Ni, Ruiyu @ 2017-09-11 13:26 UTC (permalink / raw) To: Shi, Steven, Paulo Alcantara, Laszlo Ersek, edk2-devel-01 Cc: Dong, Eric, Zeng, Star, Ard Biesheuvel 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 > > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-11 13:07 ` Shi, Steven 2017-09-11 13:26 ` Ni, Ruiyu @ 2017-09-11 13:26 ` Laszlo Ersek 2017-09-11 13:52 ` Paulo Alcantara 2 siblings, 0 replies; 34+ messages in thread From: Laszlo Ersek @ 2017-09-11 13:26 UTC (permalink / raw) To: Shi, Steven, Paulo Alcantara, edk2-devel-01 Cc: Ni, Ruiyu, Dong, Eric, Zeng, Star, Ard Biesheuvel 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 >>> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-11 13:07 ` Shi, Steven 2017-09-11 13:26 ` Ni, Ruiyu 2017-09-11 13:26 ` Laszlo Ersek @ 2017-09-11 13:52 ` Paulo Alcantara 2017-09-11 14:00 ` Shi, Steven 2 siblings, 1 reply; 34+ messages in thread From: Paulo Alcantara @ 2017-09-11 13:52 UTC (permalink / raw) To: Shi, Steven, Laszlo Ersek, edk2-devel-01 Cc: Ni, Ruiyu, Dong, Eric, Zeng, Star, Ard Biesheuvel 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 >>> > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-11 13:52 ` Paulo Alcantara @ 2017-09-11 14:00 ` Shi, Steven 0 siblings, 0 replies; 34+ messages in thread From: Shi, Steven @ 2017-09-11 14:00 UTC (permalink / raw) To: Paulo Alcantara, Laszlo Ersek, edk2-devel-01 Cc: Ni, Ruiyu, Dong, Eric, Zeng, Star, Ard Biesheuvel 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 > >>> > > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-10 0:12 [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups Laszlo Ersek ` (5 preceding siblings ...) 2017-09-10 4:24 ` [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups Shi, Steven @ 2017-09-10 15:05 ` Paulo Alcantara 2017-09-11 11:58 ` Ard Biesheuvel 2017-09-12 10:14 ` Laszlo Ersek 8 siblings, 0 replies; 34+ messages in thread From: Paulo Alcantara @ 2017-09-10 15:05 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Cc: Ard Biesheuvel, Eric Dong, Ruiyu Ni, Star Zeng 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-10 0:12 [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups Laszlo Ersek ` (6 preceding siblings ...) 2017-09-10 15:05 ` Paulo Alcantara @ 2017-09-11 11:58 ` Ard Biesheuvel 2017-09-12 10:14 ` Laszlo Ersek 8 siblings, 0 replies; 34+ messages in thread From: Ard Biesheuvel @ 2017-09-11 11:58 UTC (permalink / raw) To: Laszlo Ersek Cc: edk2-devel-01, Eric Dong, Paulo Alcantara, Ruiyu Ni, Star Zeng 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> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-10 0:12 [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups Laszlo Ersek ` (7 preceding siblings ...) 2017-09-11 11:58 ` Ard Biesheuvel @ 2017-09-12 10:14 ` Laszlo Ersek 2017-09-12 15:38 ` Ard Biesheuvel 8 siblings, 1 reply; 34+ messages in thread From: Laszlo Ersek @ 2017-09-12 10:14 UTC (permalink / raw) To: edk2-devel-01; +Cc: Ruiyu Ni, Eric Dong, Star Zeng, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-12 10:14 ` Laszlo Ersek @ 2017-09-12 15:38 ` Ard Biesheuvel 2017-09-12 21:29 ` Laszlo Ersek 0 siblings, 1 reply; 34+ messages in thread From: Ard Biesheuvel @ 2017-09-12 15:38 UTC (permalink / raw) To: Laszlo Ersek; +Cc: edk2-devel-01, Ruiyu Ni, Eric Dong, Star Zeng 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups 2017-09-12 15:38 ` Ard Biesheuvel @ 2017-09-12 21:29 ` Laszlo Ersek 0 siblings, 0 replies; 34+ messages in thread From: Laszlo Ersek @ 2017-09-12 21:29 UTC (permalink / raw) To: Ard Biesheuvel Cc: Ruiyu Ni, edk2-devel-01, Eric Dong, Star Zeng, Paulo Alcantara 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2017-09-12 21:26 UTC | newest] Thread overview: 34+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-09-10 0:12 [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups Laszlo Ersek 2017-09-10 0:13 ` [PATCH 1/5] MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA req Laszlo Ersek 2017-09-10 0:13 ` [PATCH 2/5] MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req succeeds Laszlo Ersek 2017-09-12 9:06 ` Zeng, Star 2017-09-10 0:13 ` [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem() Laszlo Ersek 2017-09-12 8:55 ` Zeng, Star 2017-09-12 9:38 ` Ni, Ruiyu 2017-09-12 9:57 ` Laszlo Ersek 2017-09-12 10:02 ` Zeng, Star 2017-09-12 9:55 ` Laszlo Ersek 2017-09-10 0:13 ` [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators Laszlo Ersek 2017-09-12 5:41 ` Bi, Dandan 2017-09-12 7:58 ` Laszlo Ersek 2017-09-12 8:28 ` Ni, Ruiyu 2017-09-12 8:46 ` Zeng, Star 2017-09-10 0:13 ` [PATCH 5/5] MdeModulePkg/PartitionDxe: remove always false comparison Laszlo Ersek 2017-09-12 8:50 ` Zeng, Star 2017-09-10 4:24 ` [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups Shi, Steven 2017-09-10 8:38 ` Laszlo Ersek 2017-09-10 13:52 ` Laszlo Ersek 2017-09-10 14:27 ` Shi, Steven 2017-09-10 15:51 ` Paulo Alcantara 2017-09-11 6:58 ` Laszlo Ersek 2017-09-11 13:55 ` Paulo Alcantara 2017-09-11 13:07 ` Shi, Steven 2017-09-11 13:26 ` Ni, Ruiyu 2017-09-11 13:26 ` Laszlo Ersek 2017-09-11 13:52 ` Paulo Alcantara 2017-09-11 14:00 ` Shi, Steven 2017-09-10 15:05 ` Paulo Alcantara 2017-09-11 11:58 ` Ard Biesheuvel 2017-09-12 10:14 ` Laszlo Ersek 2017-09-12 15:38 ` Ard Biesheuvel 2017-09-12 21:29 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox