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
