Re: [PATCH 0/2] fs/iso9660: Delay CE hop until end of current SUSP area

[Lidong Chen]

Hi,

Thanks for the detailed instruction for running the test! 
The patches look good to me. I ran the tests with and without the patches, I 
got the expected result.

Thanks,
Lidong 


> On Mar 7, 2023, at 8:56 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> SUSP 1.12 says:
> 
>  The "CE" System Use Entry indicates a Continuation Area that shall be
>  processed after the current System Use field or Continuation Area is
>  processed.
> 
> But GRUB rather takes an encountered CE entry as reason to immediately
> switch reading to the location that is given by the CE entry.
> This can skip over important information.
> 
> The usual ISO 9660 producers on GNU/Linux write the CE entry as last
> entry of System Use field or Continuation Area. So the problem does not
> show up with their output. Nevertheless, Linux and libisofs obey the
> specs whereas GRUB does not.
> 
> As demonstration i crafted a small ISO, where the CE entry comes before
> the NM entry which tells the Rock Ridge file name "RockRidgeName:x".
> Linux shows the NM name, nevertheless:
>  $ sudo mount iso9660_early_ce.iso /mnt/iso
>  mount: /mnt/iso: WARNING: source write-protected, mounted read-only.
>  $ ls /mnt/iso
>  RockRidgeName:x
>  $
> 
> GRUB does not see the NM entry and thus shows the dull ISO 9660 name
> (which is actually "ROCKRIDG.;1"):
>  $ ./grub-fstest iso9660_early_ce.iso ls /
>  rockridg
>  $
> 
> After the code change of my patch, i get:
>  $ ./grub-fstest iso9660_early_ce.iso ls /
>  RockRidgeName:x
>  $
> 
> A new code block in tests/iso9660_test.in verifies that the patched code
> is in effect:
>  make check TESTS=iso9660_test
> detects the old code state and shows that the new code still has the
> capability to cope with endless CE loops.
> 
> -------------------------------------------------------------------------
> How to create an ISO 9660 filesystem where CE is not the last SUSP entry
> of a file's directory record:
> 
>   # Deliberately chosen names
>   iso=iso9660_early_ce.iso
>   # rr_path is longer than 8, mixed-case, with non-ISO-9660 character
>   rr_path=/RockRidgeName:x
> 
>   # A dummy file as payload
>   echo x >x
> 
>   # 250 fattr characters to surely exceed the size of a directory record
>   
> long_string=0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789
> 
>   # Create ISO with the payload file and attached fattr named user.dummy .
>   # Make it small with the most restrictive ISO 9660 file name rules.
>   test -e "$iso" && rm "$iso"
>   xorriso -compliance no_emul_toc:iso_9660_level=1 \
>           -padding 0 \
>           -outdev "$iso" \
>           -xattr on \
>           -map x "$rr_path" \
>           -setfattr user.dummy "$long_string" "$rr_path" --
> 
>   # Cut out the NM field and the CE field from the directory record
>   # of $rr_path. The numbers were determined by looking at a hex dump.
>   dd if="$iso" bs=1 skip=37198 count=20 of=nm_field
>   dd if="$iso" bs=1 skip=37218 count=28 of=ce_field
> 
>   # Put them back in reverse sequence
>   dd conv=notrunc if=ce_field bs=1 seek=37198 of="$iso"
>   dd conv=notrunc if=nm_field bs=1 seek=37226 of="$iso"
> 
> -------------------------------------------------------------------------
> 
> Have a nice day :)
> 
> Thomas
> 
> Thomas Schmitt (2):
>  fs/iso9660: Delay CE hop until end of current SUSP area
>  tests: Add test for iso9660 delayed CE hop
> 
> grub-core/fs/iso9660.c        |  84 ++++++++++++++++++----------------
> tests/iso9660_early_ce.iso.gz | Bin 0 -> 709 bytes
> tests/iso9660_test.in         |  24 ++++++++++
> 3 files changed, 68 insertions(+), 40 deletions(-)
> create mode 100644 tests/iso9660_early_ce.iso.gz
> 
> --
> 2.30.2
>