public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v2 0/6] Code refinements in UdfDxe
@ 2018-10-16  8:14 Hao Wu
  2018-10-16  8:14 ` [PATCH v2 1/6] MdeModulePkg/UdfDxe: Use error handling for memory allocation failure Hao Wu
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Hao Wu @ 2018-10-16  8:14 UTC (permalink / raw)
  To: edk2-devel
  Cc: Hao Wu, Paulo Alcantara, Paulo Alcantara, Ruiyu Ni, Star Zeng,
	Jiewen Yao

Note:
Since PATCH v2 1/6 ~ 5/6 have not been changed, add the 'Reviewed-by:' tag
during the v1 series review.


V2 changes:

A. Drop PATCH v1 6/7, since removing those codes will make the function
   MangleFileName() not matching its original implementation purpose
   (according to the function description).

B. Drop change C in PATCH v1 7/7. It is reasonable for function
   SetFileInfo() to handle the expected value for the input parameters.

C. Refine the log message for PATCH v1 7/7.


V1 history:

This series will refine the codes in MdeModulePkg/Universal/Disk/UdfDxe
for:

A. Refine asserts used for memory allocation failure and error cases that
   are possible to happen. Will use error handling logic for them;

B. Address some dead codes within this module.

Cc: Paulo Alcantara <palcantara@suse.de>
Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>

Hao Wu (6):
  MdeModulePkg/UdfDxe: Use error handling for memory allocation failure
  MdeModulePkg/UdfDxe: ASSERT for false positives of NULL ptr deref
  MdeModulePkg/UdfDxe: Use error handling when fail to return LSN
  MdeModulePkg/UdfDxe: Use debug msg instead of ASSERT in UdfOpen()
  MdeModulePkg/UdfDxe: Handle dead codes in File.c
  MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c

 MdeModulePkg/Universal/Disk/UdfDxe/File.c                 |  19 ++-
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 158 +++++++++++++++-----
 2 files changed, 138 insertions(+), 39 deletions(-)

-- 
2.12.0.windows.1



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/6] MdeModulePkg/UdfDxe: Use error handling for memory allocation failure
  2018-10-16  8:14 [PATCH v2 0/6] Code refinements in UdfDxe Hao Wu
@ 2018-10-16  8:14 ` Hao Wu
  2018-10-16  8:14 ` [PATCH v2 2/6] MdeModulePkg/UdfDxe: ASSERT for false positives of NULL ptr deref Hao Wu
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Hao Wu @ 2018-10-16  8:14 UTC (permalink / raw)
  To: edk2-devel; +Cc: Hao Wu, Ruiyu Ni

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1247

For functions DuplicateFid() and DuplicateFe(), this commit will use error
handling logic instead of ASSERTs for memory allocation failure.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Reviewed-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 40 +++++++++++++++++---
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index ecc172303e..638f31bd82 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -468,8 +468,6 @@ DuplicateFid (
   *NewFileIdentifierDesc =
     (UDF_FILE_IDENTIFIER_DESCRIPTOR *)AllocateCopyPool (
       (UINTN) GetFidDescriptorLength (FileIdentifierDesc), FileIdentifierDesc);
-
-  ASSERT (*NewFileIdentifierDesc != NULL);
 }
 
 /**
@@ -490,8 +488,6 @@ DuplicateFe (
   )
 {
   *NewFileEntry = AllocateCopyPool (Volume->FileEntrySize, FileEntry);
-
-  ASSERT (*NewFileEntry != NULL);
 }
 
 /**
@@ -1370,7 +1366,15 @@ InternalFindFile (
     }
 
     DuplicateFe (BlockIo, Volume, Parent->FileEntry, &File->FileEntry);
+    if (File->FileEntry == NULL) {
+      return EFI_OUT_OF_RESOURCES;
+    }
+
     DuplicateFid (Parent->FileIdentifierDesc, &File->FileIdentifierDesc);
+    if (File->FileIdentifierDesc == NULL) {
+      FreePool (File->FileEntry);
+      return EFI_OUT_OF_RESOURCES;
+    }
 
     return EFI_SUCCESS;
   }
@@ -1732,9 +1736,20 @@ FindFile (
         // We've already a file pointer (Root) for the root directory. Duplicate
         // its FE/EFE and FID descriptors.
         //
-        DuplicateFe (BlockIo, Volume, Root->FileEntry, &File->FileEntry);
-        DuplicateFid (Root->FileIdentifierDesc, &File->FileIdentifierDesc);
         Status = EFI_SUCCESS;
+        DuplicateFe (BlockIo, Volume, Root->FileEntry, &File->FileEntry);
+        if (File->FileEntry == NULL) {
+          Status = EFI_OUT_OF_RESOURCES;
+        } else {
+          //
+          // File->FileEntry is not NULL.
+          //
+          DuplicateFid (Root->FileIdentifierDesc, &File->FileIdentifierDesc);
+          if (File->FileIdentifierDesc == NULL) {
+            FreePool (File->FileEntry);
+            Status = EFI_OUT_OF_RESOURCES;
+          }
+        }
       }
     } else {
       //
@@ -1874,6 +1889,9 @@ ReadDirectoryEntry (
   } while (FileIdentifierDesc->FileCharacteristics & DELETED_FILE);
 
   DuplicateFid (FileIdentifierDesc, FoundFid);
+  if (*FoundFid == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
 
   return EFI_SUCCESS;
 }
@@ -2031,8 +2049,18 @@ ResolveSymlink (
       // "." (current file). Duplicate both FE/EFE and FID of this file.
       //
       DuplicateFe (BlockIo, Volume, PreviousFile.FileEntry, &File->FileEntry);
+      if (File->FileEntry == NULL) {
+        Status = EFI_OUT_OF_RESOURCES;
+        goto Error_Find_File;
+      }
+
       DuplicateFid (PreviousFile.FileIdentifierDesc,
                     &File->FileIdentifierDesc);
+      if (File->FileIdentifierDesc == NULL) {
+        FreePool (File->FileEntry);
+        Status = EFI_OUT_OF_RESOURCES;
+        goto Error_Find_File;
+      }
       goto Next_Path_Component;
     case 5:
       //
-- 
2.12.0.windows.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/6] MdeModulePkg/UdfDxe: ASSERT for false positives of NULL ptr deref
  2018-10-16  8:14 [PATCH v2 0/6] Code refinements in UdfDxe Hao Wu
  2018-10-16  8:14 ` [PATCH v2 1/6] MdeModulePkg/UdfDxe: Use error handling for memory allocation failure Hao Wu
@ 2018-10-16  8:14 ` Hao Wu
  2018-10-16  8:14 ` [PATCH v2 3/6] MdeModulePkg/UdfDxe: Use error handling when fail to return LSN Hao Wu
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Hao Wu @ 2018-10-16  8:14 UTC (permalink / raw)
  To: edk2-devel; +Cc: Hao Wu, Ruiyu Ni

This commit adds ASSERTs to address false positive reports of NULL
pointer dereference issues raised from static analysis with regard to
function ReadDirectoryEntry().

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Reviewed-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/File.c                 | 9 +++++++++
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 6f07bf2066..2249f4ea0e 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -408,6 +408,15 @@ UdfRead (
 
         goto Done;
       }
+      //
+      // After calling function ReadDirectoryEntry(), if 'NewFileIdentifierDesc'
+      // is NULL, then the 'Status' must be EFI_OUT_OF_RESOURCES. Hence, if the
+      // code reaches here, 'NewFileIdentifierDesc' must be not NULL.
+      //
+      // The ASSERT here is for addressing a false positive NULL pointer
+      // dereference issue raised from static analysis.
+      //
+      ASSERT (NewFileIdentifierDesc != NULL);
 
       if (!IS_FID_PARENT_FILE (NewFileIdentifierDesc)) {
         break;
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 638f31bd82..8b58cc9eb1 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -1404,6 +1404,15 @@ InternalFindFile (
 
       break;
     }
+    //
+    // After calling function ReadDirectoryEntry(), if 'FileIdentifierDesc' is
+    // NULL, then the 'Status' must be EFI_OUT_OF_RESOURCES. Hence, if the code
+    // reaches here, 'FileIdentifierDesc' must be not NULL.
+    //
+    // The ASSERT here is for addressing a false positive NULL pointer
+    // dereference issue raised from static analysis.
+    //
+    ASSERT (FileIdentifierDesc != NULL);
 
     if (FileIdentifierDesc->FileCharacteristics & PARENT_FILE) {
       //
-- 
2.12.0.windows.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/6] MdeModulePkg/UdfDxe: Use error handling when fail to return LSN
  2018-10-16  8:14 [PATCH v2 0/6] Code refinements in UdfDxe Hao Wu
  2018-10-16  8:14 ` [PATCH v2 1/6] MdeModulePkg/UdfDxe: Use error handling for memory allocation failure Hao Wu
  2018-10-16  8:14 ` [PATCH v2 2/6] MdeModulePkg/UdfDxe: ASSERT for false positives of NULL ptr deref Hao Wu
@ 2018-10-16  8:14 ` Hao Wu
  2018-10-16  8:14 ` [PATCH v2 4/6] MdeModulePkg/UdfDxe: Use debug msg instead of ASSERT in UdfOpen() Hao Wu
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Hao Wu @ 2018-10-16  8:14 UTC (permalink / raw)
  To: edk2-devel; +Cc: Hao Wu, Ruiyu Ni

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1248

This commit will refine the ASSERTs in function GetLongAdLsn() and
GetAllocationDescriptorLsn() when the logical sector number cannot be
returned due to unrecognized media format.

Error handling logic will be used for those ASSERTs.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Reviewed-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 101 +++++++++++++-------
 1 file changed, 69 insertions(+), 32 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 8b58cc9eb1..1400846bf1 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -271,26 +271,39 @@ GetPdFromLongAd (
 /**
   Return logical sector number of a given Long Allocation Descriptor.
 
-  @param[in]  Volume              Volume information pointer.
-  @param[in]  LongAd              Long Allocation Descriptor pointer.
+  @param[in]   Volume             Volume information pointer.
+  @param[in]   LongAd             Long Allocation Descriptor pointer.
+  @param[out]  Lsn                Logical sector number pointer.
 
-  @return The logical sector number of a given Long Allocation Descriptor.
+  @retval EFI_SUCCESS             Logical sector number successfully returned.
+  @retval EFI_UNSUPPORTED         Logical sector number is not returned due to
+                                  unrecognized format.
 
 **/
-UINT64
+EFI_STATUS
 GetLongAdLsn (
-  IN UDF_VOLUME_INFO                 *Volume,
-  IN UDF_LONG_ALLOCATION_DESCRIPTOR  *LongAd
+  IN  UDF_VOLUME_INFO                 *Volume,
+  IN  UDF_LONG_ALLOCATION_DESCRIPTOR  *LongAd,
+  OUT UINT64                          *Lsn
   )
 {
   UDF_PARTITION_DESCRIPTOR *PartitionDesc;
 
   PartitionDesc = GetPdFromLongAd (Volume, LongAd);
-  ASSERT (PartitionDesc != NULL);
+  if (PartitionDesc == NULL) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: Fail to get the Partition Descriptor from the given Long Allocation Descriptor.\n",
+      __FUNCTION__
+      ));
+    return EFI_UNSUPPORTED;
+  }
 
-  return (UINT64)PartitionDesc->PartitionStartingLocation -
-    Volume->MainVdsStartLocation +
-    LongAd->ExtentLocation.LogicalBlockNumber;
+  *Lsn = (UINT64)PartitionDesc->PartitionStartingLocation -
+         Volume->MainVdsStartLocation +
+         LongAd->ExtentLocation.LogicalBlockNumber;
+
+  return EFI_SUCCESS;
 }
 
 /**
@@ -342,7 +355,10 @@ FindFileSetDescriptor (
   UDF_DESCRIPTOR_TAG             *DescriptorTag;
 
   LogicalVolDesc = &Volume->LogicalVolDesc;
-  Lsn = GetLongAdLsn (Volume, &LogicalVolDesc->LogicalVolumeContentsUse);
+  Status = GetLongAdLsn (Volume, &LogicalVolDesc->LogicalVolumeContentsUse, &Lsn);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
 
   //
   // As per UDF 2.60 specification:
@@ -732,34 +748,43 @@ GetAllocationDescriptor (
   @param[in]  Volume              Volume information pointer.
   @param[in]  ParentIcb           Long Allocation Descriptor pointer.
   @param[in]  Ad                  Allocation Descriptor pointer.
+  @param[out] Lsn                 Logical sector number pointer.
 
-  @return The logical sector number of the given Allocation Descriptor.
+  @retval EFI_SUCCESS             Logical sector number of the given Allocation
+                                  Descriptor successfully returned.
+  @retval EFI_UNSUPPORTED         Logical sector number of the given Allocation
+                                  Descriptor is not returned due to unrecognized
+                                  format.
 
 **/
-UINT64
+EFI_STATUS
 GetAllocationDescriptorLsn (
-  IN UDF_FE_RECORDING_FLAGS          RecordingFlags,
-  IN UDF_VOLUME_INFO                 *Volume,
-  IN UDF_LONG_ALLOCATION_DESCRIPTOR  *ParentIcb,
-  IN VOID                            *Ad
+  IN  UDF_FE_RECORDING_FLAGS          RecordingFlags,
+  IN  UDF_VOLUME_INFO                 *Volume,
+  IN  UDF_LONG_ALLOCATION_DESCRIPTOR  *ParentIcb,
+  IN  VOID                            *Ad,
+  OUT UINT64                          *Lsn
   )
 {
   UDF_PARTITION_DESCRIPTOR *PartitionDesc;
 
   if (RecordingFlags == LongAdsSequence) {
-    return GetLongAdLsn (Volume, (UDF_LONG_ALLOCATION_DESCRIPTOR *)Ad);
+    return GetLongAdLsn (Volume, (UDF_LONG_ALLOCATION_DESCRIPTOR *)Ad, Lsn);
   } else if (RecordingFlags == ShortAdsSequence) {
     PartitionDesc = GetPdFromLongAd (Volume, ParentIcb);
-    ASSERT (PartitionDesc != NULL);
+    if (PartitionDesc == NULL) {
+      return EFI_UNSUPPORTED;
+    }
 
-    return GetShortAdLsn (
-      Volume,
-      PartitionDesc,
-      (UDF_SHORT_ALLOCATION_DESCRIPTOR *)Ad
-      );
+    *Lsn = GetShortAdLsn (
+             Volume,
+             PartitionDesc,
+             (UDF_SHORT_ALLOCATION_DESCRIPTOR *)Ad
+             );
+    return EFI_SUCCESS;
   }
 
-  return 0;
+  return EFI_UNSUPPORTED;
 }
 
 /**
@@ -804,10 +829,14 @@ GetAedAdsOffset (
   UDF_DESCRIPTOR_TAG                *DescriptorTag;
 
   ExtentLength  = GET_EXTENT_LENGTH (RecordingFlags, Ad);
-  Lsn           = GetAllocationDescriptorLsn (RecordingFlags,
+  Status        = GetAllocationDescriptorLsn (RecordingFlags,
                                               Volume,
                                               ParentIcb,
-                                              Ad);
+                                              Ad,
+                                              &Lsn);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
 
   Data = AllocatePool (ExtentLength);
   if (Data == NULL) {
@@ -1156,10 +1185,14 @@ ReadFile (
 
       ExtentLength = GET_EXTENT_LENGTH (RecordingFlags, Ad);
 
-      Lsn = GetAllocationDescriptorLsn (RecordingFlags,
-                                        Volume,
-                                        ParentIcb,
-                                        Ad);
+      Status = GetAllocationDescriptorLsn (RecordingFlags,
+                                           Volume,
+                                           ParentIcb,
+                                           Ad,
+                                           &Lsn);
+      if (EFI_ERROR (Status)) {
+        goto Done;
+      }
 
       switch (ReadFileInfo->Flags) {
       case ReadFileGetFileSize:
@@ -1630,7 +1663,11 @@ FindFileEntry (
   UDF_DESCRIPTOR_TAG  *DescriptorTag;
   VOID                *ReadBuffer;
 
-  Lsn               = GetLongAdLsn (Volume, Icb);
+  Status = GetLongAdLsn (Volume, Icb, &Lsn);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
   LogicalBlockSize  = Volume->LogicalVolDesc.LogicalBlockSize;
 
   ReadBuffer = AllocateZeroPool (Volume->FileEntrySize);
-- 
2.12.0.windows.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 4/6] MdeModulePkg/UdfDxe: Use debug msg instead of ASSERT in UdfOpen()
  2018-10-16  8:14 [PATCH v2 0/6] Code refinements in UdfDxe Hao Wu
                   ` (2 preceding siblings ...)
  2018-10-16  8:14 ` [PATCH v2 3/6] MdeModulePkg/UdfDxe: Use error handling when fail to return LSN Hao Wu
@ 2018-10-16  8:14 ` Hao Wu
  2018-10-16  8:14 ` [PATCH v2 5/6] MdeModulePkg/UdfDxe: Handle dead codes in File.c Hao Wu
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Hao Wu @ 2018-10-16  8:14 UTC (permalink / raw)
  To: edk2-devel; +Cc: Hao Wu, Ruiyu Ni

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1248

Within function UdfOpen():
This commit will use debug messages instead of using ASSERT when an error
occurs after calling GetFileSize().

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Reviewed-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 2249f4ea0e..0730e6a3fa 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -257,8 +257,12 @@ UdfOpen (
     &NewPrivFileData->File,
     &NewPrivFileData->FileSize
     );
-  ASSERT_EFI_ERROR (Status);
   if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: GetFileSize() fails with status - %r.\n",
+      __FUNCTION__, Status
+      ));
     goto Error_Get_File_Size;
   }
 
-- 
2.12.0.windows.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 5/6] MdeModulePkg/UdfDxe: Handle dead codes in File.c
  2018-10-16  8:14 [PATCH v2 0/6] Code refinements in UdfDxe Hao Wu
                   ` (3 preceding siblings ...)
  2018-10-16  8:14 ` [PATCH v2 4/6] MdeModulePkg/UdfDxe: Use debug msg instead of ASSERT in UdfOpen() Hao Wu
@ 2018-10-16  8:14 ` Hao Wu
  2018-10-16  8:14 ` [PATCH v2 6/6] MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c Hao Wu
  2018-10-17  2:48 ` [PATCH v2 0/6] Code refinements in UdfDxe Zeng, Star
  6 siblings, 0 replies; 8+ messages in thread
From: Hao Wu @ 2018-10-16  8:14 UTC (permalink / raw)
  To: edk2-devel; +Cc: Hao Wu, Ruiyu Ni

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1249

We found potential dead codes within File.c during the code coverage test.

After manual review, we think the below ones are positive reports:

A. In function UdfRead():
  else if (IS_FID_DELETED_FILE (Parent->FileIdentifierDesc)) {
    Status = EFI_DEVICE_ERROR;
  }

A File Identifier Descriptor will be get from the UDF media only by
function ReadDirectoryEntry(). And within this function, all the File
Identifier Descriptor with 'DELETED_FILE' characteristics will be skipped
and will not be returned. Hence, the above codes in function UdfRead()
will never be hit.

This commit will add "ASSERT (FALSE);" before the above line to indicate
this.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Reviewed-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/File.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 0730e6a3fa..eb7d79692b 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -500,6 +500,10 @@ UdfRead (
     PrivFileData->FilePosition++;
     Status = EFI_SUCCESS;
   } else if (IS_FID_DELETED_FILE (Parent->FileIdentifierDesc)) {
+    //
+    // Code should never reach here.
+    //
+    ASSERT (FALSE);
     Status = EFI_DEVICE_ERROR;
   }
 
-- 
2.12.0.windows.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 6/6] MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c
  2018-10-16  8:14 [PATCH v2 0/6] Code refinements in UdfDxe Hao Wu
                   ` (4 preceding siblings ...)
  2018-10-16  8:14 ` [PATCH v2 5/6] MdeModulePkg/UdfDxe: Handle dead codes in File.c Hao Wu
@ 2018-10-16  8:14 ` Hao Wu
  2018-10-17  2:48 ` [PATCH v2 0/6] Code refinements in UdfDxe Zeng, Star
  6 siblings, 0 replies; 8+ messages in thread
From: Hao Wu @ 2018-10-16  8:14 UTC (permalink / raw)
  To: edk2-devel
  Cc: Hao Wu, Paulo Alcantara, Paulo Alcantara, Ruiyu Ni, Star Zeng,
	Jiewen Yao

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1249

We found potential dead codes within File.c during the code coverage test.

After manual review, we think the below ones are positive reports:

A. For function GetAllocationDescriptor():
Due to the all the calling places for this function, the input parameter
'RecordingFlags' can only with value 'LongAdsSequence' or
'ShortAdsSequence'. Moreover, this is also mentioned in the function
description comments for GetAllocationDescriptor().

So the below code will never be reached:

  return EFI_DEVICE_ERROR;

This commit will add "ASSERT (FALSE);" before the above line to indicate
this and thus matching the function description comments.

B. For function GetAllocationDescriptorLsn():
Due to the all the calling places for this function, the input parameter
'RecordingFlags' can only with value 'LongAdsSequence' or
'ShortAdsSequence'. Moreover, this is also mentioned in the function
description comments for GetAllocationDescriptorLsn().

So the below code will never be reached:

  return EFI_UNSUPPORTED;

This commit will add "ASSERT (FALSE);" before the above line to indicate
this and thus matching the function description comments.

Cc: Paulo Alcantara <palcantara@suse.de>
Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 1400846bf1..9048a18f64 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -738,6 +738,10 @@ GetAllocationDescriptor (
       );
   }
 
+  //
+  // Code should never reach here.
+  //
+  ASSERT (FALSE);
   return EFI_DEVICE_ERROR;
 }
 
@@ -784,6 +788,10 @@ GetAllocationDescriptorLsn (
     return EFI_SUCCESS;
   }
 
+  //
+  // Code should never reach here.
+  //
+  ASSERT (FALSE);
   return EFI_UNSUPPORTED;
 }
 
-- 
2.12.0.windows.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/6] Code refinements in UdfDxe
  2018-10-16  8:14 [PATCH v2 0/6] Code refinements in UdfDxe Hao Wu
                   ` (5 preceding siblings ...)
  2018-10-16  8:14 ` [PATCH v2 6/6] MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c Hao Wu
@ 2018-10-17  2:48 ` Zeng, Star
  6 siblings, 0 replies; 8+ messages in thread
From: Zeng, Star @ 2018-10-17  2:48 UTC (permalink / raw)
  To: Wu, Hao A, edk2-devel@lists.01.org
  Cc: Paulo Alcantara, Paulo Alcantara, Ni, Ruiyu, Yao, Jiewen,
	Zeng, Star

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


Thanks,
Star
-----Original Message-----
From: Wu, Hao A 
Sent: Tuesday, October 16, 2018 4:15 PM
To: edk2-devel@lists.01.org
Cc: Wu, Hao A <hao.a.wu@intel.com>; Paulo Alcantara <palcantara@suse.de>; Paulo Alcantara <paulo@paulo.ac>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: [PATCH v2 0/6] Code refinements in UdfDxe

Note:
Since PATCH v2 1/6 ~ 5/6 have not been changed, add the 'Reviewed-by:' tag during the v1 series review.


V2 changes:

A. Drop PATCH v1 6/7, since removing those codes will make the function
   MangleFileName() not matching its original implementation purpose
   (according to the function description).

B. Drop change C in PATCH v1 7/7. It is reasonable for function
   SetFileInfo() to handle the expected value for the input parameters.

C. Refine the log message for PATCH v1 7/7.


V1 history:

This series will refine the codes in MdeModulePkg/Universal/Disk/UdfDxe
for:

A. Refine asserts used for memory allocation failure and error cases that
   are possible to happen. Will use error handling logic for them;

B. Address some dead codes within this module.

Cc: Paulo Alcantara <palcantara@suse.de>
Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>

Hao Wu (6):
  MdeModulePkg/UdfDxe: Use error handling for memory allocation failure
  MdeModulePkg/UdfDxe: ASSERT for false positives of NULL ptr deref
  MdeModulePkg/UdfDxe: Use error handling when fail to return LSN
  MdeModulePkg/UdfDxe: Use debug msg instead of ASSERT in UdfOpen()
  MdeModulePkg/UdfDxe: Handle dead codes in File.c
  MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c

 MdeModulePkg/Universal/Disk/UdfDxe/File.c                 |  19 ++-
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 158 +++++++++++++++-----
 2 files changed, 138 insertions(+), 39 deletions(-)

--
2.12.0.windows.1



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-10-17  2:48 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-16  8:14 [PATCH v2 0/6] Code refinements in UdfDxe Hao Wu
2018-10-16  8:14 ` [PATCH v2 1/6] MdeModulePkg/UdfDxe: Use error handling for memory allocation failure Hao Wu
2018-10-16  8:14 ` [PATCH v2 2/6] MdeModulePkg/UdfDxe: ASSERT for false positives of NULL ptr deref Hao Wu
2018-10-16  8:14 ` [PATCH v2 3/6] MdeModulePkg/UdfDxe: Use error handling when fail to return LSN Hao Wu
2018-10-16  8:14 ` [PATCH v2 4/6] MdeModulePkg/UdfDxe: Use debug msg instead of ASSERT in UdfOpen() Hao Wu
2018-10-16  8:14 ` [PATCH v2 5/6] MdeModulePkg/UdfDxe: Handle dead codes in File.c Hao Wu
2018-10-16  8:14 ` [PATCH v2 6/6] MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c Hao Wu
2018-10-17  2:48 ` [PATCH v2 0/6] Code refinements in UdfDxe Zeng, Star

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox