[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 Nikita Melnichenko changed: What|Removed |Added Latest Commit|https://commits.kde.org/kru |https://commits.kde.org/kru |sader/c33f9629c7e313be9e8bd |sader/16f54c8e1b7a4c223288f |de0579e00a5f98fad39 |7f183e5b36286689097 --- Comment #15 from Nikita Melnichenko --- Git commit 16f54c8e1b7a4c223288f7f183e5b36286689097 by Nikita Melnichenko. Committed on 22/06/2018 at 07:57. Pushed by melnichenko into branch '2.7'. KIso: Fixed file offsets of the underlying IO device The patch is proposed by Theo . kio_iso uses KArchive's KCompressionDevice to access the ISO file. The device is created in KIso::prepareDevice and KCompressionDevice's constructor creates a QIODevice and a QFileDevice. The corresponding *Private devices have separate sets of member variables 'pos' and 'devicePos' that get out of sync when the QFileDevice is closed at the end of KIso::openArchive. The QFileDevice is reset to offset zero while QIODevice keeps the offset positions it has after reading the ISO file system. When the ISO file is opened for reading the file data, QFileDevice's offset positions are treated as if they haven't been reset and subsequent seeks fall short of the requested offset. The workaround is suggested by Qt documentation [1] "When subclassing QIODevice, you must call QIODevice::seek() at the start of your function to ensure integrity with QIODevice's built-in buffer." For more details see the discussion in the bug. FIXED: [ 372023 ] ISO files listing/extracting broken Differential Revision: https://phabricator.kde.org/D13626 [1] http://doc.qt.io/qt-5/qiodevice.html#seek (cherry picked from commit c33f9629c7e313be9e8bdde0579e00a5f98fad39) M +7-1iso/iso.cpp M +4-0iso/kiso.cpp https://commits.kde.org/krusader/16f54c8e1b7a4c223288f7f183e5b36286689097 -- You are receiving this mail because: You are watching all bug changes.
[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 Nikita Melnichenko changed: What|Removed |Added Status|ASSIGNED|RESOLVED Latest Commit||https://commits.kde.org/kru ||sader/c33f9629c7e313be9e8bd ||de0579e00a5f98fad39 Resolution|--- |FIXED --- Comment #14 from Nikita Melnichenko --- Git commit c33f9629c7e313be9e8bdde0579e00a5f98fad39 by Nikita Melnichenko. Committed on 22/06/2018 at 07:44. Pushed by melnichenko into branch 'master'. KIso: Fixed file offsets of the underlying IO device The patch is proposed by Theo . kio_iso uses KArchive's KCompressionDevice to access the ISO file. The device is created in KIso::prepareDevice and KCompressionDevice's constructor creates a QIODevice and a QFileDevice. The corresponding *Private devices have separate sets of member variables 'pos' and 'devicePos' that get out of sync when the QFileDevice is closed at the end of KIso::openArchive. The QFileDevice is reset to offset zero while QIODevice keeps the offset positions it has after reading the ISO file system. When the ISO file is opened for reading the file data, QFileDevice's offset positions are treated as if they haven't been reset and subsequent seeks fall short of the requested offset. The workaround is suggested by Qt documentation [1] "When subclassing QIODevice, you must call QIODevice::seek() at the start of your function to ensure integrity with QIODevice's built-in buffer." For more details see the discussion in the bug. FIXED: [ 372023 ] ISO files listing/extracting broken Differential Revision: https://phabricator.kde.org/D13626 [1] http://doc.qt.io/qt-5/qiodevice.html#seek M +7-1iso/iso.cpp M +4-0iso/kiso.cpp https://commits.kde.org/krusader/c33f9629c7e313be9e8bdde0579e00a5f98fad39 -- You are receiving this mail because: You are watching all bug changes.
[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 --- Comment #13 from Theo --- (In reply to Nikita Melnichenko from comment #10) > If you feel there is a bug in > KCompressionDevice implementation, could you please file a bug against the > product "frameworks-karchive" to discuss with the devs? Here it is: https://bugs.kde.org/show_bug.cgi?id=395471 -- You are receiving this mail because: You are watching all bug changes.
[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 --- Comment #12 from Nikita Melnichenko --- FYI: https://phabricator.kde.org/D13626 -- You are receiving this mail because: You are watching all bug changes.
[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 --- Comment #11 from Theo --- (In reply to Theo from comment #9) > The following workaround fixes both the incomplete file listing > and the wrong file data on the first try (makes my previously posted fix > unnecessary): Insert > > dev->seek(0); I'm afraid I was wrong about that. This only fixes the directory listing. For the correct file data on the first try the previous fix from comment #8 is required. (I think I accidentally kept the changes from the first fix while testing the second fix.) -- You are receiving this mail because: You are watching all bug changes.
[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 Nikita Melnichenko changed: What|Removed |Added Assignee|krusader-bugs-n...@kde.org |nikita+...@melnichenko.name Keywords||reproducible Status|CONFIRMED |ASSIGNED --- Comment #10 from Nikita Melnichenko --- Theo, thanks for sharing all your investigation results and a potential fix. (In reply to Theo from comment #8) > What would this setup roughly look like? Using my distributor's > debuginfo/debugsource packages and investigating with GDB Some people hesitate to do this and to compile the modified code. It's great that you can do it by yourself! > I start to believe that KCompressionDevice is broken, and unless someone > explains to me why I'm wrong I doubt we have KArchive experts here. If you feel there is a bug in KCompressionDevice implementation, could you please file a bug against the product "frameworks-karchive" to discuss with the devs? This may help other apps too. Some thoughts why KCompressionDevice might have been used instead of a plain file. If you look into KIso::KIso and KIso::prepareDevice, there is a code to guess mime type and take a different action. AFAIK, ISO format doesn't support any internal compression, so I guess it's for supporting .iso.gz, .iso.bz2 on the fly. > Insert dev->seek(0); The workaround seems to be harmless as there is another seek that follows. I'll check more and prepare a code review for the team when I have the right amount of free time. -- You are receiving this mail because: You are watching all bug changes.
[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 --- Comment #9 from Theo --- (In reply to Theo from comment #8) > This does not fix the issue of missing files in the directory listing, > compare for instance the contents of '/ISO9660' and '/El Torito BootJoliet > level 3' when browsing sgd_cdrom_1.30.iso as archive. This is caused by the same problem: KCompressionDevice and its QFile, both derived from QIODevice, have member variables 'pos' and 'devicePos' that get out of sync. The following workaround fixes both the incomplete file listing and the wrong file data on the first try (makes my previously posted fix unnecessary): Insert dev->seek(0); before if (dev->seek((qint64)start << (qint64)11)) { if ((dev->read(buf, len << 11u)) != -1) return (len); } in the callback function 'readf' in kiso.cpp. This seek is done in KCompressionDevice::seek(qint64 pos) only in some cases, see kcompressiondevice.cpp[1]: if (d->deviceReadPos < pos) { // we can start from here [...] } else { // we have to start from 0 ! Ugly and slow, but better than the previous // solution (KTarGz was allocating everything into memory) if (!seek(0)) { // recursive return false; } [...] } Apparently, this is considered ugly, but I start to believe that KCompressionDevice is broken, and unless someone explains to me why I'm wrong and how KCompressionDevice is supposed to work I see no other way to fix this (again, I'm not a developer and might have no clue what I'm talking about). Ceterum censeo KCompressionDevice should not be used on uncompressed ISO files anyway: https://bugs.kde.org/show_bug.cgi?id=395296 [1] https://api.kde.org/frameworks/karchive/html/kcompressiondevice_8cpp_source.html#l00178 -- You are receiving this mail because: You are watching all bug changes.
[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 --- Comment #8 from Theo --- (In reply to Nikita Melnichenko from comment #7) > if you feel like you can > help figuring out the reason, you're more than welcome to do this. I can > help you with the debugging setup, etc. What would this setup roughly look like? Using my distributor's debuginfo/debugsource packages and investigating with GDB, I can say the following so far (note that I might not really know what I'm doing here, and that I can provide details on my findings if necessary): kio_iso uses KArchive's KCompressionDevice[1] to access the ISO file. The device is created in KIso::prepareDevice and KCompressionDevice's constructor creates a QIODevice and a QFileDevice. The corresponding *Private devices have separate sets of member variables 'pos' and 'devicePos' that get out of sync when the QFileDevice is closed at the end of KIso::openArchive. The QFileDevice is reset to offset zero while QIODevice keeps the offset positions it has after reading the ISO file system. When the ISO file is opened for reading the file data, QFileDevice's offset positions are treated as if they haven't been reset and subsequent seeks fall short of the requested offset. Now I don't know whether QIODevice's offset should be reset when the QFileDevice is closed after reading the file system or when it is opened for reading the requested file data, but I tried the latter after reading the following in the Qt documentation: "When subclassing QIODevice, you must call QIODevice::seek() at the start of your function to ensure integrity with QIODevice's built-in buffer."[2] Replacing if (size && !m_isoFile->device()->isOpen()) m_isoFile->device()->open(QIODevice::ReadOnly); by if (size && !m_isoFile->device()->isOpen()) { m_isoFile->device()->open(QIODevice::ReadOnly); m_isoFile->device()->seek(0); } in kio_isoProtocol::getFile (iso/iso.cpp) seems to work and I get the correct file data on the first try. This does not fix the issue of missing files in the directory listing, compare for instance the contents of '/ISO9660' and '/El Torito BootJoliet level 3' when browsing sgd_cdrom_1.30.iso as archive. Regarding my question about reading through a lot of data when requesting file data, it seems that the problem here is kio_iso's use of KCompressionDevice for uncompressed ISO files. KCompressionDevice does not seem to support random access and a seek is implemented as a read from the beginning, even for uncompressed files. This seems like a really stupid idea since ISO files tend to be big and should be read with a method that supports random access. [1] https://api.kde.org/frameworks/karchive/html/classKCompressionDevice.html [2] http://doc.qt.io/qt-5/qiodevice.html#seek -- You are receiving this mail because: You are watching all bug changes.
[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 Nikita Melnichenko changed: What|Removed |Added Keywords||triaged --- Comment #7 from Nikita Melnichenko --- Theo, thanks for providing the details, it will help with debugging. Indeed, I completely missed that I needed to read file contents. I confirm on my end that viewing files on the first try shows binary garbage. I think we don't have people who know the code of kio_iso and ISO format very well (original dev is retired long ago), so if you feel like you can help figuring out the reason, you're more than welcome to do this. I can help you with the debugging setup, etc. -- You are receiving this mail because: You are watching all bug changes.
[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 --- Comment #6 from Theo --- (In reply to Nikita Melnichenko from comment #5) > (In reply to Theo from comment #4) > > It happens with every ISO image I try. I did a test with a small sample size > > of ISO images and the data offset seems to be off by the same sector count > > on the first try for a given ISO. I got 52, 64, and 80 2K sectors ahead for > > three different CDs/DVDs. > > How did you determine this? By brute force. I opened a file on the ISO with Okteta using iso:/, copied the first 16 bytes of binary garbage in hex format, then fed this into a binary grep on the whole ISO image to get the offset. The same for the correct data after reloading the file. I did this for more than one file for every tested ISO image. I use https://github.com/tmbinc/bgrep for the binary grep, but it should also be possible to use standard grep (or some other method), albeit not as fast: https://stackoverflow.com/questions/4180081/binary-grep-on-linux/17168777#17168777 Example: $ okteta iso:/home/user/sgd_cdrom_1.30.iso/ISO9660/grubdetect.lua $ bgrep -A 20 C78508FFC7850CFF sgd_cdrom_1.30.iso | sed 's/\\x//g' sgd_cdrom_1.30.iso: 00042800 c78508ffc7850cff0080 sgd_cdrom_1.30.iso: 00042872 c78508ffc7850cff In this case I have to use "-A" to check that the first match is the correct one. After reloading the file in Okteta: $ bgrep 23216C75610A0A66756E6374696F6E20 sgd_cdrom_1.30.iso sgd_cdrom_1.30.iso: 0005c800 $ echo 'ibase=16;(0005C800-00042800)/800' | bc -l 52. > > For instance, for > > > > https://sourceforge.net/projects/supergrub2/files/1.30/ > > > > kio_iso seems to deliver data that is 52 sectors ahead on the first try. > > This image is not identified as ISO in my krusader (v2.7). Could this be related to this annoyance: https://bugs.freedesktop.org/show_bug.cgi?id=99421#c20? I get this nonsense: $ kmimetypefinder5 sgd_cdrom_1.30.iso model/x.stl-binary > Dolphin > recognizes it fine and reads from the first try. For example, I can browse to > > iso:/.../sgd_cdrom_1.30.iso/El Torito BootJoliet level 3/boot/grub/ > > without a problem. If the link is directly copied to Krusader, it also opens > the link fine. Doesn't make a difference whether I use Krusader or any other KDE program. It seems to depend only on the iso:/ protocol. Did you also try opening a file on the ISO to check if the data is correct? There might also be a problem with some missing files in the directory listing (comment #0). > > By the way, is it normal that a lot of data is read when I access a small > > file on a big ISO image? > > I'm not sure as I'm not familiar with the format. It's unexpected because one should get the position of the file from the file system on the ISO and then go directly to the file without reading a lot of data from the image. But maybe I have a completely wrong idea about how ISO 9606 works... -- You are receiving this mail because: You are watching all bug changes.
[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 --- Comment #5 from Nikita Melnichenko --- (In reply to Theo from comment #4) > (In reply to Nikita Melnichenko from comment #3) > > What's the example of such a file? The smaller the better. > It happens with every ISO image I try. I did a test with a small sample size > of ISO images and the data offset seems to be off by the same sector count > on the first try for a given ISO. I got 52, 64, and 80 2K sectors ahead for > three different CDs/DVDs. How did you determine this? This may be useful for later debugging. > > For instance, for > > https://sourceforge.net/projects/supergrub2/files/1.30/ > > kio_iso seems to deliver data that is 52 sectors ahead on the first try. This image is not identified as ISO in my krusader (v2.7). Dolphin recognizes it fine and reads from the first try. For example, I can browse to iso:/.../sgd_cdrom_1.30.iso/El Torito BootJoliet level 3/boot/grub/ without a problem. If the link is directly copied to Krusader, it also opens the link fine. > > By the way, is it normal that a lot of data is read when I access a small > file on a big ISO image? I'm not sure as I'm not familiar with the format. -- You are receiving this mail because: You are watching all bug changes.
[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 --- Comment #4 from Theo --- (In reply to Nikita Melnichenko from comment #3) > What's the example of such a file? The smaller the better. It happens with every ISO image I try. I did a test with a small sample size of ISO images and the data offset seems to be off by the same sector count on the first try for a given ISO. I got 52, 64, and 80 2K sectors ahead for three different CDs/DVDs. For instance, for https://sourceforge.net/projects/supergrub2/files/1.30/ kio_iso seems to deliver data that is 52 sectors ahead on the first try. By the way, is it normal that a lot of data is read when I access a small file on a big ISO image? -- You are receiving this mail because: You are watching all bug changes.
[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 Nikita Melnichenko changed: What|Removed |Added CC||nikita+...@melnichenko.name --- Comment #3 from Nikita Melnichenko --- What's the example of such a file? The smaller the better. I can't repro so far. -- You are receiving this mail because: You are watching all bug changes.
[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 Theo changed: What|Removed |Added CC||alpha0...@yahoo.de --- Comment #2 from Theo --- Can confirm for kio_iso 2.6.0-2.1 from openSUSE Tumbleweed. I don't know about missing files, but I get binary garbage on the first try opening a file. Reloading the opened file (for instance by pressing F5 in KrViewer, KWrite, or Okteta) gives me the correct data of the file. -- You are receiving this mail because: You are watching all bug changes.
[krusader] [Bug 372023] ISO files listing/extracting broken
https://bugs.kde.org/show_bug.cgi?id=372023 Alex Bikadorov changed: What|Removed |Added CC||alex.bikado...@kdemail.net Status|UNCONFIRMED |CONFIRMED Ever confirmed|0 |1 --- Comment #1 from Alex Bikadorov --- I'm not familiar with ISO. When opened with the iso:/ protocol the first level are always three directories: >/El Torito Boot >/El Torito BootJoliet level 3 >/ISO9660 The actual content is in one of these and Ark shows this content directly. And yes, opening any of the files shows garbage. Maybe an encoding issue. Windows/Microsoft ISOs cannot be be opened with Krusader OR Ark. Content is one README file: > This disc contains a "UDF" file system and requires an operating system > that supports the ISO-13346 "UDF" file system specification. But I don't have an ISO with missing files. Can you give an example? -- You are receiving this mail because: You are watching all bug changes.