Re: [PATCH v6 0/6] read-only UDF file system support

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 8 September 2017 at 20:40, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/08/17 21:21, Ard Biesheuvel wrote:
>> On 8 September 2017 at 19:47, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 09/08/17 14:41, Paulo Alcantara wrote:
>>>> Hi,
>>>>
>>>> This series introduces read-only UDF file system support in EDK2. As
>>>> Laszlo (or Red Hat) seemed to be interested in such support, I'm posting
>>>> it again after ~3 years.
>>>>
>>>> The idea is not replacing the default FAT file system, nor breaking any
>>>> existing file system support, but extending EDK2 with a new file system
>>>> that might be useful for some people who are looking for specific file
>>>> system features that current FAT doesn't support.
>>>>
>>>> Originally the driver was written to support UDF file systems as
>>>> specified by OSTA Universal Disk Format Specification 2.60. However,
>>>> some Windows 10 Enterprise ISO (UDF bridge) images that I tested
>>>> supported a revision of 1.02 thus I had to rework the driver a little
>>>> bit to support such revision as well.
>>>>
>>>> v2:
>>>>  - Rework to _partially_ support UDF revisions <2.60.
>>>>  - Use existing CDROM_VOLUME_DESCRIPTOR structure defined in Eltorito.h
>>>>    instead of creating another one (UDF_VOLUME_DESCRIPTOR).
>>>>  - Fixed UdfDxe to correctly follow UEFI driver model.
>>>>  - Use HARDDRIVE_DEVICE_PATH instead of a vendor-defined one.
>>>>  - Detect UDF file systems only in PartitionDxe, and let UdfDxe driver
>>>>    check for specific UDF device path to decide whether or not install
>>>>    SimpleFs protocol.
>>>>  - Place MdePkg changes in a separate patch.
>>>> v3:
>>>>  - Install UDF partition child handles with a Vendor-Defined Media
>>>>    Device Path.
>>>>  - Changed UdfDxe to check for Vendor-Defined Media Device Paths with a
>>>>    specific UDF file system GUID when determining to whether or not
>>>>    start the driver.
>>>>  - Removed leading TAB chars in some source files identified by
>>>>    PatchCheck.py tool.
>>>> v4:
>>>>  - Added missing R-b's.
>>>> v5:
>>>>  - Fixed OVMF IA32 build.
>>>>  - Fixed a typo in UdfDriveBindingStop() ("This" -> "SimpleFs") which
>>>>    broke retrieval of private fs data from SimpleFs protocol --
>>>>    identified by 'reconnect -r' command in UEFI shell.
>>>> v6:
>>>>  - Fixed a bug in UdfRead() that'd pontentially break in ARM or IA32
>>>>    by allowing caller to read more than 4GiB of data
>>>>    (i.e. BufferSize pointer is dereferenced as an UINT64 * and it's
>>>>     followed by 4 bytes that are nonzero).
>>>>
>>>> Repo:   https://github.com/pcacjr/edk2.git
>>>> Branch: udf-fs-v6
>>>>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>> Cc: Andrew Fish <afish@apple.com>
>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>> Cc: Star Zeng <star.zeng@intel.com>
>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>> Cc: Mark Doran <mark.doran@intel.com>
>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>> Cc: hao.a.wu@intel.com
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Paulo Alcantara <pcacjr@zytor.com>
>>>> ---
>>>>
>>>> Paulo Alcantara (6):
>>>>   MdePkg: Add UDF volume structure definitions
>>>>   MdeModulePkg/PartitionDxe: Add UDF file system support
>>>>   MdeModulePkg: Initial UDF/ECMA-167 file system support
>>>>   OvmfPkg: Enable UDF file system support
>>>>   ArmVirtPkg: Enable UDF file system support
>>>>   Nt32Pkg: Enable UDF file system support
>>>>
>>>>  ArmVirtPkg/ArmVirtQemu.dsc                         |    3 +-
>>>>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc               |    3 +-
>>>>  ArmVirtPkg/ArmVirtQemuKernel.dsc                   |    3 +-
>>>>  ArmVirtPkg/ArmVirtXen.dsc                          |    3 +-
>>>>  ArmVirtPkg/ArmVirtXen.fdf                          |    1 +
>>>>  .../Universal/Disk/PartitionDxe/Partition.c        |    9 +-
>>>>  .../Universal/Disk/PartitionDxe/Partition.h        |   32 +-
>>>>  .../Universal/Disk/PartitionDxe/PartitionDxe.inf   |    3 +-
>>>>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     |  318 +++
>>>>  MdeModulePkg/Universal/Disk/UdfDxe/ComponentName.c |  185 ++
>>>>  MdeModulePkg/Universal/Disk/UdfDxe/File.c          |  908 ++++++++
>>>>  MdeModulePkg/Universal/Disk/UdfDxe/FileName.c      |  195 ++
>>>>  .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 2447 ++++++++++++++++++++
>>>>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |  344 +++
>>>>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           | 1244 ++++++++++
>>>>  MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf      |   66 +
>>>>  MdePkg/Include/IndustryStandard/Udf.h              |   60 +
>>>>  Nt32Pkg/Nt32Pkg.dsc                                |    1 +
>>>>  Nt32Pkg/Nt32Pkg.fdf                                |    1 +
>>>>  OvmfPkg/OvmfPkgIa32.dsc                            |    1 +
>>>>  OvmfPkg/OvmfPkgIa32.fdf                            |    1 +
>>>>  OvmfPkg/OvmfPkgIa32X64.dsc                         |    1 +
>>>>  OvmfPkg/OvmfPkgIa32X64.fdf                         |    1 +
>>>>  OvmfPkg/OvmfPkgX64.dsc                             |    1 +
>>>>  OvmfPkg/OvmfPkgX64.fdf                             |    1 +
>>>>  25 files changed, 5821 insertions(+), 11 deletions(-)
>>>>  create mode 100644 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
>>>>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/ComponentName.c
>>>>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/File.c
>>>>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/FileName.c
>>>>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>>>>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/Udf.c
>>>>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/Udf.h
>>>>  create mode 100644 MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
>>>>  create mode 100644 MdePkg/Include/IndustryStandard/Udf.h
>>>>
>>>
>>> Pushed as commit range 7aee391fa3d0..b696c64d4fc3.
>>>
>>
>> This code breaks the Clang build:
>>
>> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:937:11:
>> error: variable 'Status' is used uninitialized whenever 'if' condition
>> is false [-Werror,-Wsometimes-uninitialized]
>>       if (ReadFileInfo->FileData == NULL) {
>>           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:1148:10:
>> note: uninitialized use occurs here
>>   return Status;
>>          ^~~~~~
>> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:937:7:
>> note: remove the 'if' if its condition is always true
>>       if (ReadFileInfo->FileData == NULL) {
>>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:932:16:
>> error: variable 'Status' is used uninitialized whenever 'if' condition
>> is false [-Werror,-Wsometimes-uninitialized]
>>     } else if (ReadFileInfo->Flags == READ_FILE_ALLOCATE_AND_READ) {
>>                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:1148:10:
>> note: uninitialized use occurs here
>>   return Status;
>>          ^~~~~~
>> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:932:12:
>> note: remove the 'if' if its condition is always true
>>     } else if (ReadFileInfo->Flags == READ_FILE_ALLOCATE_AND_READ) {
>>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:930:9:
>> error: variable 'Status' is used uninitialized whenever 'if' condition
>> is true [-Werror,-Wsometimes-uninitialized]
>>     if (ReadFileInfo->Flags == READ_FILE_GET_FILESIZE) {
>>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:1148:10:
>> note: uninitialized use occurs here
>>   return Status;
>>          ^~~~~~
>> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:930:5:
>> note: remove the 'if' if its condition is always false
>>     if (ReadFileInfo->Flags == READ_FILE_GET_FILESIZE) {
>>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:869:33:
>> note: initialize the variable 'Status' to silence this warning
>>   EFI_STATUS              Status;
>>                                 ^
>>                                  = 0
>> 3 errors generated.
>
> Thanks for the report -- this was sort of expected, given the size of
> the driver. My test builds for IA32/X64/ARM/AARCH64 with gcc-4.8/GCC48
> and gcc-6.1.1/GCC5 didn't catch the above.
>
> I think if these issues were fixed, the build wouldn't complete
> immediately; there are likely other instances.
>
> Could you please recommend a build command line, and perhaps clang
> package names that are "usual" on a few GNU/Linux distros, to Paulo?
> Then he could set up a virtual machine (or even install clang natively),
> and keep writing fixes until the build finishes.
>
> I'm curious how many of the above warnings will uncover real bugs, and
> how many will need suppression.
>

To me, it looks like the diagnostic is correct, and we should
initialize Status to EFI_SUCCESS at the start of the function. That is
the only breakage with clang-3.8