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

[Daniel Kiper]

On Fri, Mar 31, 2023 at 12:57:05AM +0000, Glenn Washburn wrote:
> On 3/30/23 18:30, Daniel Kiper wrote:
> > On Tue, Mar 07, 2023 at 05:56:49PM +0100, Thomas Schmitt 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"
> >
> > Patch set LGTM. Though I would prefer to hear Glenn's opinion on the test
> > Thomas introduces because AIUI this [1] email presented some concerns.
> >
> > Glenn?
> >
> > Daniel
> >
> > [1] https://lists.gnu.org/archive/html/grub-devel/2023-03/msg00022.html
>
> I had partially written a response to the above referenced email a couple
> weeks ago, but didn't sent it because it seemed unnecessary with the way
> Thomas changed the test to work with or without extra trailing spaces. So
> the concerns in the referenced email aren't a concern for this test patch.
>
> As for a more general concern, not really either. If the trailing spaces are
> removed later on, tests will fail and then they can be fixed. To sum up what
> I was going to say in response to the referenced email, grub-fstest is a
> tool for running grub commands/code in user-space and has subcommands
> corresponding for specifying what code to run. I haven't verified, but I
> believe that in this case the grub "ls" command is creating the output, so
> its likely that the command is what is producing the extra space, not
> grub-fstest itself. Again, not something to worry about here, IMO.
>
> So patch #2, the test patch, in the series LGTM.

Glenn, thank you for detailed response.

Then for both patches Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Thomas, thank you for fixing this issue.

Daniel