Re: [Libcdio-devel] Rock Ridge and libisofs/xorriso 'AL' extension

[Thomas Schmitt]

Hi,

here are my preliminary sunday morning comments. I will send a new patch
(planned for today) which addresses them.

------------------------------------------------------------------

The problematic code gesture exists several times in iso9660_fs.c.
You now changed an occurence in _fs_stat_traverse() which i did not
encounter while investigating.

The one which i encountered during stepping with gdb is in
function iso9660_fs_readdir(): 
  
http://git.savannah.gnu.org/cgit/libcdio.git/tree/lib/iso9660/iso9660_fs.c?h=TS-RockRidge-Fix&id=7db119850977f5fa37d6d5e8ed14c18e7fa2df13#n1316

The code gestures look very similar. This calls for a common function
which checks for zero directory length.
It may also do the check for block crossing which i propose below.

Still existing in the git branch are the "offset++" gestures at:

  
http://git.savannah.gnu.org/cgit/libcdio.git/tree/lib/iso9660/iso9660_fs.c?h=TS-RockRidge-Fix&id=7db119850977f5fa37d6d5e8ed14c18e7fa2df13#n1103
  
http://git.savannah.gnu.org/cgit/libcdio.git/tree/lib/iso9660/iso9660_fs.c?h=TS-RockRidge-Fix&id=7db119850977f5fa37d6d5e8ed14c18e7fa2df13#n1318
  
http://git.savannah.gnu.org/cgit/libcdio.git/tree/lib/iso9660/iso9660_fs.c?h=TS-RockRidge-Fix&id=7db119850977f5fa37d6d5e8ed14c18e7fa2df13#n1401
  
http://git.savannah.gnu.org/cgit/libcdio.git/tree/lib/iso9660/iso9660_fs.c?h=TS-RockRidge-Fix&id=7db119850977f5fa37d6d5e8ed14c18e7fa2df13#n1613

I'd say they all need the same safety cap.

------------------------------------------------------------------------

You did not interpret my experimental #ifdefs correctly:

> http://git.savannah.gnu.org/cgit/libcdio.git/commit/?h=TS-RockRidge-Fix&id=7db119850977f5fa37d6d5e8ed14c18e7fa2df13

> +     offset += ISO_BLOCKSIZE - (offset % ISO_BLOCKSIZE);
>       offset++;

The line "offset++" was to be replaced by the "offset+=" line.
(It was in the #else-case of the #ifdef in my test proposal.)

As it is now, it would skip the first byte in the next block
and thus misread any directory which consists of more than one block.

So:

-       offset++

But as said, this should become a call to a new function, anyways.

------------------------------------------------------------------------

> + libisofs of the libburnia project uses a directory
> + record length of 0 as an indicator to advance to the next block

The Linux kernel does it too.
Probably that's the stronger reference to quote.


> > 1:
> > libcdio did not notice that the mistaken directory record (caused by the
> > stray byte with value 128) reached over the end of the current block.
> > ...
> > 2:
> > libcdio seems to assume a valid chain of SUSP fields when it encounters
> > additional space after the end of ISO 9660 file name and possible padding

> My understanding is that these two issues still would need resolution.

Yes. My proposal was meant only to fix the memory corruption caused
by the bad ISO of bug 45015 without discriminating good ISOs if they
contain non-Rock-Ridge SUSO fields.

Fixing problem 2 is not without the risk to exclude half-good ISOs
which for some reason have Rock Ridge without announcing it properly.
Linux is similarly tolerant than libcdio.

So possibly one will only address problem 1 to raise the propbability
to catch fully bad ISOs before they can cause havoc in the code.

I will include this in my proposal of a common function to check all
five occasions of the handling of directory block ends.


I'll begin now with implementation and testing.
This time without deceiving macro alternatives (they help while one
is unsure whether one does the right thing).

What is the preferred form of an applicable patch ?
Mail inline ? Attachment ?

Must i clone the the git branch TS-RockRidge-Fix first ?
(git still gives me creeps. It is so optimistic about merging.)


Have a nice day :)

Thomas