[PATCH v1 0/7] Code refinements in UdfDxe

[PATCH v1 0/7] Code refinements in UdfDxe

[Hao Wu]

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 <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>

Hao Wu (7):
  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: Remove dead codes in FileName.c
  MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c

 MdeModulePkg/Universal/Disk/UdfDxe/File.c                 |  19 ++-
 MdeModulePkg/Universal/Disk/UdfDxe/FileName.c             |  15 --
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 162 +++++++++++++++-----
 3 files changed, 142 insertions(+), 54 deletions(-)

-- 
2.12.0.windows.1

[PATCH v1 1/7] MdeModulePkg/UdfDxe: Use error handling for memory allocation failure

[Hao Wu]

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: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@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 | 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

[PATCH v1 2/7] MdeModulePkg/UdfDxe: ASSERT for false positives of NULL ptr deref

[Hao Wu]

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

Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@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

[PATCH v1 3/7] MdeModulePkg/UdfDxe: Use error handling when fail to return LSN

[Hao Wu]

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: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@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 | 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

[PATCH v1 4/7] MdeModulePkg/UdfDxe: Use debug msg instead of ASSERT in UdfOpen()

[Hao Wu]

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: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@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

[PATCH v1 5/7] MdeModulePkg/UdfDxe: Handle dead codes in File.c

[Hao Wu]

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: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@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

[PATCH v1 6/7] MdeModulePkg/UdfDxe: Remove dead codes in FileName.c

[Hao Wu]

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 MangleFileName():

  FileName = TrimString (FileName);
  // Begin of dead codes
  if (*FileName == L'\0') {
    goto Exit;
  }
  // End of dead codes

When the code reaches the TrimString() call, the string in 'FileName' is
guaranteed to have a '\' character due to the call patterns of
MangleFileName(). So after trimming the lead-off/tailing white spaces,
string in 'FileName' will not be an empty string.

B. In function MangleFileName():

  if (FileName[0] == L'.') {
    if (FileName[1] == L'.') {
      if (FileName[2] == L'\0') {
        goto Exit;
      } else {
        FileName += 2;
      }
    } else if (FileName[1] == L'\0') {
      goto Exit;
    }
  }

When the code hits the above checks, string in 'FileName' will always have
a leading '\' character (denoting an absolute path) due to the call
patterns of MangleFileName(). So no leading '.' can be there in string
'FileName'.

This commit will remove those dead codes.

Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileName.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileName.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileName.c
index 36551a4dba..18549e4e45 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileName.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileName.c
@@ -128,9 +128,6 @@ MangleFileName (
   }
 
   FileName = TrimString (FileName);
-  if (*FileName == L'\0') {
-    goto Exit;
-  }
 
   if ((StrLen (FileName) > 1) && (FileName[StrLen (FileName) - 1] == L'\\')) {
     FileName[StrLen (FileName) - 1] = L'\0';
@@ -138,18 +135,6 @@ MangleFileName (
 
   FileNameSavedPointer = FileName;
 
-  if (FileName[0] == L'.') {
-    if (FileName[1] == L'.') {
-      if (FileName[2] == L'\0') {
-        goto Exit;
-      } else {
-        FileName += 2;
-      }
-    } else if (FileName[1] == L'\0') {
-      goto Exit;
-    }
-  }
-
   while (*FileName != L'\0') {
     if (*FileName == L'\\') {
       FileName = ExcludeTrailingBackslashes (FileName);
-- 
2.12.0.windows.1

[PATCH v1 7/7] MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c

[Hao Wu]

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'.

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.

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'.

So the below code will never be reached:

  return EFI_UNSUPPORTED;

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

C. For function SetFileInfo():
Due to the all the calling places for this function, the input parameter
'FileName' will never be a NULL pointer.

So the below codes will never be reached:

  } else {
    FileInfo->FileName[0] = '\0';
  }

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

Cc: Paulo Alcantara <paulo@paulo.ac>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 1400846bf1..19acd0554c 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;
 }
 
@@ -2413,6 +2421,10 @@ SetFileInfo (
   if (FileName != NULL) {
     StrCpyS (FileInfo->FileName, StrLen (FileName) + 1, FileName);
   } else {
+    //
+    // Code should never reach here.
+    //
+    ASSERT (FALSE);
     FileInfo->FileName[0] = '\0';
   }
 
-- 
2.12.0.windows.1

Re: [PATCH v1 0/7] Code refinements in UdfDxe

[Paulo Alcantara]

Hi,

Hao Wu <hao.a.wu@intel.com> writes:

> 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 <paulo@paulo.ac>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
>
> Hao Wu (7):
>   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: Remove dead codes in FileName.c
>   MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c
>
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c                 |  19 ++-
>  MdeModulePkg/Universal/Disk/UdfDxe/FileName.c             |  15 --
>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 162 +++++++++++++++-----
>  3 files changed, 142 insertions(+), 54 deletions(-)

Looks good to me. Thanks!

For the series:

Reviewed-by: Paulo Alcantara <palcantara@suse.de>

	Paulo

Re: [PATCH v1 6/7] MdeModulePkg/UdfDxe: Remove dead codes in FileName.c

[Leif Lindholm]

On Mon, Oct 15, 2018 at 12:55:21PM +0800, Hao Wu wrote:
> 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 MangleFileName():
> 
>   FileName = TrimString (FileName);
>   // Begin of dead codes
>   if (*FileName == L'\0') {
>     goto Exit;
>   }
>   // End of dead codes
> 
> When the code reaches the TrimString() call, the string in 'FileName' is
> guaranteed to have a '\' character due to the call patterns of

What in the call pattern guarantees this? Please be precise.

> MangleFileName(). So after trimming the lead-off/tailing white spaces,
> string in 'FileName' will not be an empty string.
> 
> B. In function MangleFileName():
> 
>   if (FileName[0] == L'.') {
>     if (FileName[1] == L'.') {
>       if (FileName[2] == L'\0') {
>         goto Exit;
>       } else {
>         FileName += 2;
>       }
>     } else if (FileName[1] == L'\0') {
>       goto Exit;
>     }
>   }
> 
> When the code hits the above checks, string in 'FileName' will always have
> a leading '\' character (denoting an absolute path) due to the call
> patterns of MangleFileName(). So no leading '.' can be there in string
> 'FileName'.

What in the call pattern guarantees this? Please be precise.

Regards,

Leif

Re: [PATCH v1 0/7] Code refinements in UdfDxe

[Zeng, Star]

On 2018/10/15 12:55, Hao Wu wrote:
> 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 <paulo@paulo.ac>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> 
> Hao Wu (7):
>    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: Remove dead codes in FileName.c
>    MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c
> 
>   MdeModulePkg/Universal/Disk/UdfDxe/File.c                 |  19 ++-
>   MdeModulePkg/Universal/Disk/UdfDxe/FileName.c             |  15 --
>   MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 162 +++++++++++++++-----
>   3 files changed, 142 insertions(+), 54 deletions(-)

Hao,

Thanks for the patches.

Reviewed-by: Star Zeng <star.zeng@intel.com> to patch 1 ~ 6.
Some feedback will be sent for patch 7.

Star

> 

Re: [PATCH v1 6/7] MdeModulePkg/UdfDxe: Remove dead codes in FileName.c

[Wu, Hao A]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Leif Lindholm
> Sent: Tuesday, October 16, 2018 2:20 PM
> To: Wu, Hao A
> Cc: Ni, Ruiyu; edk2-devel@lists.01.org; Zeng, Star
> Subject: Re: [edk2] [PATCH v1 6/7] MdeModulePkg/UdfDxe: Remove dead
> codes in FileName.c
> 
> On Mon, Oct 15, 2018 at 12:55:21PM +0800, Hao Wu wrote:
> > 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 MangleFileName():
> >
> >   FileName = TrimString (FileName);
> >   // Begin of dead codes
> >   if (*FileName == L'\0') {
> >     goto Exit;
> >   }
> >   // End of dead codes
> >
> > When the code reaches the TrimString() call, the string in 'FileName' is
> > guaranteed to have a '\' character due to the call patterns of
> 
> What in the call pattern guarantees this? Please be precise.

OK, I will update the log message.

> 
> > MangleFileName(). So after trimming the lead-off/tailing white spaces,
> > string in 'FileName' will not be an empty string.
> >
> > B. In function MangleFileName():
> >
> >   if (FileName[0] == L'.') {
> >     if (FileName[1] == L'.') {
> >       if (FileName[2] == L'\0') {
> >         goto Exit;
> >       } else {
> >         FileName += 2;
> >       }
> >     } else if (FileName[1] == L'\0') {
> >       goto Exit;
> >     }
> >   }
> >
> > When the code hits the above checks, string in 'FileName' will always have
> > a leading '\' character (denoting an absolute path) due to the call
> > patterns of MangleFileName(). So no leading '.' can be there in string
> > 'FileName'.
> 
> What in the call pattern guarantees this? Please be precise.

OK, I will update the log message.

Thanks for the comments.

Best Regards,
Hao Wu

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

Re: [PATCH v1 7/7] MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c

[Zeng, Star]

On 2018/10/15 12:55, Hao Wu wrote:
> 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'.
> 
> 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.
> 
> 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'.
> 
> So the below code will never be reached:
> 
>    return EFI_UNSUPPORTED;
> 
> This commit will add "ASSERT (FALSE);" before the above line to indicate
> this.
> 
> C. For function SetFileInfo():
> Due to the all the calling places for this function, the input parameter
> 'FileName' will never be a NULL pointer.
> 
> So the below codes will never be reached:
> 
>    } else {
>      FileInfo->FileName[0] = '\0';
>    }
> 
> This commit will add "ASSERT (FALSE);" before the above lines to indicate
> this.

Hao,

Thanks for the patch.

I think we should see what is the expected value for the parameter, but 
not see how caller uses the parameter.
 From this point of view, I think the C case is valid and may be no need 
to change.


Thanks,
Star

> 
> Cc: Paulo Alcantara <paulo@paulo.ac>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@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 | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> index 1400846bf1..19acd0554c 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;
>   }
>   
> @@ -2413,6 +2421,10 @@ SetFileInfo (
>     if (FileName != NULL) {
>       StrCpyS (FileInfo->FileName, StrLen (FileName) + 1, FileName);
>     } else {
> +    //
> +    // Code should never reach here.
> +    //
> +    ASSERT (FALSE);
>       FileInfo->FileName[0] = '\0';
>     }
>   
> 

Re: [PATCH v1 6/7] MdeModulePkg/UdfDxe: Remove dead codes in FileName.c

[Zeng, Star]

Hao,

If these code removing will make the function to be not matched with its 
original implementation purpose according to the function description, 
and the description is not updated, the code's readability will be 
sacrificed. I prefer to keep the code's readability and follow the 
function's original implementation purpose.


Thanks,
Star

On 2018/10/16 14:28, Wu, Hao A wrote:
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Leif Lindholm
>> Sent: Tuesday, October 16, 2018 2:20 PM
>> To: Wu, Hao A
>> Cc: Ni, Ruiyu; edk2-devel@lists.01.org; Zeng, Star
>> Subject: Re: [edk2] [PATCH v1 6/7] MdeModulePkg/UdfDxe: Remove dead
>> codes in FileName.c
>>
>> On Mon, Oct 15, 2018 at 12:55:21PM +0800, Hao Wu wrote:
>>> 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 MangleFileName():
>>>
>>>    FileName = TrimString (FileName);
>>>    // Begin of dead codes
>>>    if (*FileName == L'\0') {
>>>      goto Exit;
>>>    }
>>>    // End of dead codes
>>>
>>> When the code reaches the TrimString() call, the string in 'FileName' is
>>> guaranteed to have a '\' character due to the call patterns of
>>
>> What in the call pattern guarantees this? Please be precise.
> 
> OK, I will update the log message.
> 
>>
>>> MangleFileName(). So after trimming the lead-off/tailing white spaces,
>>> string in 'FileName' will not be an empty string.
>>>
>>> B. In function MangleFileName():
>>>
>>>    if (FileName[0] == L'.') {
>>>      if (FileName[1] == L'.') {
>>>        if (FileName[2] == L'\0') {
>>>          goto Exit;
>>>        } else {
>>>          FileName += 2;
>>>        }
>>>      } else if (FileName[1] == L'\0') {
>>>        goto Exit;
>>>      }
>>>    }
>>>
>>> When the code hits the above checks, string in 'FileName' will always have
>>> a leading '\' character (denoting an absolute path) due to the call
>>> patterns of MangleFileName(). So no leading '.' can be there in string
>>> 'FileName'.
>>
>> What in the call pattern guarantees this? Please be precise.
> 
> OK, I will update the log message.
> 
> Thanks for the comments.
> 
> Best Regards,
> Hao Wu
> 
>>
>> Regards,
>>
>> Leif
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v1 7/7] MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c

[Zeng, Star]

On 2018/10/16 14:59, Zeng, Star wrote:
> On 2018/10/15 12:55, Hao Wu wrote:
>> 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'.
>>
>> 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.
>>
>> 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'.
>>
>> So the below code will never be reached:
>>
>>    return EFI_UNSUPPORTED;
>>
>> This commit will add "ASSERT (FALSE);" before the above line to indicate
>> this.
>>
>> C. For function SetFileInfo():
>> Due to the all the calling places for this function, the input parameter
>> 'FileName' will never be a NULL pointer.
>>
>> So the below codes will never be reached:
>>
>>    } else {
>>      FileInfo->FileName[0] = '\0';
>>    }
>>
>> This commit will add "ASSERT (FALSE);" before the above lines to indicate
>> this.
> 
> Hao,
> 
> Thanks for the patch.
> 
> I think we should see what is the expected value for the parameter, but 
> not see how caller uses the parameter.
>  From this point of view, I think the C case is valid and may be no need 
> to change.

More information about the C case. There are two places in the function 
to handle FileName == NULL, but this patch only updates one place. If 
the patch wants to forbid the function to accept FileName == NULL, it 
should update those two places and update function description at the 
same time. Otherwise keep the function no change.

Thanks,
Star

> 
> 
> Thanks,
> Star
> 
>>
>> Cc: Paulo Alcantara <paulo@paulo.ac>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@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 | 12 
>> ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
>> b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>> index 1400846bf1..19acd0554c 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;
>>   }
>> @@ -2413,6 +2421,10 @@ SetFileInfo (
>>     if (FileName != NULL) {
>>       StrCpyS (FileInfo->FileName, StrLen (FileName) + 1, FileName);
>>     } else {
>> +    //
>> +    // Code should never reach here.
>> +    //
>> +    ASSERT (FALSE);
>>       FileInfo->FileName[0] = '\0';
>>     }
>>

Re: [PATCH v1 7/7] MdeModulePkg/UdfDxe: Handle dead codes in FileSystemOperations.c

[Wu, Hao A]

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, October 16, 2018 3:50 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu; Zeng, Star
> Subject: Re: [edk2] [PATCH v1 7/7] MdeModulePkg/UdfDxe: Handle dead
> codes in FileSystemOperations.c
> 
> On 2018/10/16 14:59, Zeng, Star wrote:
> > On 2018/10/15 12:55, Hao Wu wrote:
> >> 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'.
> >>
> >> 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.
> >>
> >> 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'.
> >>
> >> So the below code will never be reached:
> >>
> >>    return EFI_UNSUPPORTED;
> >>
> >> This commit will add "ASSERT (FALSE);" before the above line to indicate
> >> this.
> >>
> >> C. For function SetFileInfo():
> >> Due to the all the calling places for this function, the input parameter
> >> 'FileName' will never be a NULL pointer.
> >>
> >> So the below codes will never be reached:
> >>
> >>    } else {
> >>      FileInfo->FileName[0] = '\0';
> >>    }
> >>
> >> This commit will add "ASSERT (FALSE);" before the above lines to indicate
> >> this.
> >
> > Hao,
> >
> > Thanks for the patch.
> >
> > I think we should see what is the expected value for the parameter, but
> > not see how caller uses the parameter.
> >  From this point of view, I think the C case is valid and may be no need
> > to change.
> 
> More information about the C case. There are two places in the function
> to handle FileName == NULL, but this patch only updates one place. If
> the patch wants to forbid the function to accept FileName == NULL, it
> should update those two places and update function description at the
> same time. Otherwise keep the function no change.
> 

Got it. I will update the series accordingly.

Best Regards,
Hao Wu

> Thanks,
> Star
> 
> >
> >
> > Thanks,
> > Star
> >
> >>
> >> Cc: Paulo Alcantara <paulo@paulo.ac>
> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >> Cc: Star Zeng <star.zeng@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 | 12
> >> ++++++++++++
> >>   1 file changed, 12 insertions(+)
> >>
> >> diff --git
> a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> >> b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> >> index 1400846bf1..19acd0554c 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;
> >>   }
> >> @@ -2413,6 +2421,10 @@ SetFileInfo (
> >>     if (FileName != NULL) {
> >>       StrCpyS (FileInfo->FileName, StrLen (FileName) + 1, FileName);
> >>     } else {
> >> +    //
> >> +    // Code should never reach here.
> >> +    //
> >> +    ASSERT (FALSE);
> >>       FileInfo->FileName[0] = '\0';
> >>     }
> >>

Re: [PATCH v1 6/7] MdeModulePkg/UdfDxe: Remove dead codes in FileName.c

[Wu, Hao A]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Zeng, Star
> Sent: Tuesday, October 16, 2018 3:46 PM
> To: Wu, Hao A; Leif Lindholm
> Cc: Ni, Ruiyu; edk2-devel@lists.01.org; Zeng, Star
> Subject: Re: [edk2] [PATCH v1 6/7] MdeModulePkg/UdfDxe: Remove dead
> codes in FileName.c
> 
> Hao,
> 
> If these code removing will make the function to be not matched with its
> original implementation purpose according to the function description,
> and the description is not updated, the code's readability will be
> sacrificed. I prefer to keep the code's readability and follow the
> function's original implementation purpose.
> 

OK, I will drop this patch in the next series.

Best Regards,
Hao Wu

> 
> Thanks,
> Star
> 
> On 2018/10/16 14:28, Wu, Hao A wrote:
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> >> Leif Lindholm
> >> Sent: Tuesday, October 16, 2018 2:20 PM
> >> To: Wu, Hao A
> >> Cc: Ni, Ruiyu; edk2-devel@lists.01.org; Zeng, Star
> >> Subject: Re: [edk2] [PATCH v1 6/7] MdeModulePkg/UdfDxe: Remove
> dead
> >> codes in FileName.c
> >>
> >> On Mon, Oct 15, 2018 at 12:55:21PM +0800, Hao Wu wrote:
> >>> 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 MangleFileName():
> >>>
> >>>    FileName = TrimString (FileName);
> >>>    // Begin of dead codes
> >>>    if (*FileName == L'\0') {
> >>>      goto Exit;
> >>>    }
> >>>    // End of dead codes
> >>>
> >>> When the code reaches the TrimString() call, the string in 'FileName' is
> >>> guaranteed to have a '\' character due to the call patterns of
> >>
> >> What in the call pattern guarantees this? Please be precise.
> >
> > OK, I will update the log message.
> >
> >>
> >>> MangleFileName(). So after trimming the lead-off/tailing white spaces,
> >>> string in 'FileName' will not be an empty string.
> >>>
> >>> B. In function MangleFileName():
> >>>
> >>>    if (FileName[0] == L'.') {
> >>>      if (FileName[1] == L'.') {
> >>>        if (FileName[2] == L'\0') {
> >>>          goto Exit;
> >>>        } else {
> >>>          FileName += 2;
> >>>        }
> >>>      } else if (FileName[1] == L'\0') {
> >>>        goto Exit;
> >>>      }
> >>>    }
> >>>
> >>> When the code hits the above checks, string in 'FileName' will always
> have
> >>> a leading '\' character (denoting an absolute path) due to the call
> >>> patterns of MangleFileName(). So no leading '.' can be there in string
> >>> 'FileName'.
> >>
> >> What in the call pattern guarantees this? Please be precise.
> >
> > OK, I will update the log message.
> >
> > Thanks for the comments.
> >
> > Best Regards,
> > Hao Wu
> >
> >>
> >> Regards,
> >>
> >> Leif
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel