<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/122579/">https://git.reviewboard.kde.org/r/122579/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Februar 15th, 2015, 10:37 nachm. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Nice, love unittests.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Most of the new code in kzip.cpp is a copy of the contents of the previous while loop, though.
Is there any chance for extracting this into a helper method, to avoid the duplication? It would facilitate maintenance in the long run, especially if more changes around this are needed.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I resisted any refactoring... but if you ask for it... ;)
Updated the diff to include some for the code that is affected by duplication. Should also bring a fix the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">compr_size > dev->size()</code> branch, as that one so far did not do the check if the checked 4-byte block was not across another token start. So at least in theory that could have resulted in missed header tokens, no? Also improved that logic slightly and only do seeking back as much as needed if another <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">P</code> was found.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Did not go the extra mile though to also cover the code for getting to start of file if the file does not start with any ZIP header, as that one only PK\003\004. Improvement with seeking only as much as needed if another <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">P</code> is found is left for a add-on commit, if this patch is okay-ed.</p></pre>
<br />
<p>- Friedrich W. H.</p>
<br />
<p>On Februar 16th, 2015, Mitternacht UTC, Friedrich W. H. Kossebau wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks and David Faure.</div>
<div>By Friedrich W. H. Kossebau.</div>
<p style="color: grey;"><i>Updated Feb. 16, 2015, Mitternacht</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
karchive
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Currently the parsing code of KZip assumes that there are only data descriptors behind the file data if bit 3 of the general purpose bit flag in the local file header is set. But, the spec does not forbid the data descriptors also to be used when that bit is not set, see the "SHOULD" (not "MUST") in 4.3.9.1 of the ZIP spec (https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT):</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"> <span style="color: #666666">4.3.9.1</span> This descriptor MUST exist <span style="color: #008000; font-weight: bold">if</span> bit <span style="color: #666666">3</span> of the general
purpose bit flag is set (see below). It is byte aligned
and immediately follows the last byte of compressed data.
This descriptor SHOULD be used only when it was not possible to
seek in the output .ZIP file, e.g., when the output .ZIP file
was standard output or a non<span style="color: #666666">-</span>seekable device. For ZIP64(tm) format
archives, the compressed and uncompressed sizes are <span style="color: #666666">8</span> bytes each.
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch fixes that by testing for a data descriptor behind the file data. It tests both for data descriptors with and without the PK78 signature, as 4.3.9.3 of the ZIP spec recommends it:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"> <span style="color: #666666">4.3.9.3</span> Although not originally assigned a signature, the value
<span style="color: #666666">0x08074b50</span> has commonly been adopted as a signature value
<span style="color: #008000; font-weight: bold">for</span> the data descriptor record. Implementers should be
aware that ZIP files may be encountered with or without this
signature marking data descriptors and SHOULD account <span style="color: #008000; font-weight: bold">for</span>
either <span style="color: #008000; font-weight: bold">case</span> when reading ZIP files to ensure compatibility.
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The patch also comes with a unit test and two files where such redundant data descriptors are used, once with and once without signature (hand crafted, using "zip" and Okteta :) ).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Motivation:
Currently Calligra Words cannot open ODT files as created by the ODT export of DokuWiki, while at least LibreOffice can and also all the zip tools have no problem with the file. You can create such ODT files e.g. on http://plugtest.opendocsociety.org/</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have also started to prepare a patch against kdelibs 4.14 and will complete it, once this RR has passed review.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">All KArchive tests pass, Calligra can load the ODT files created by DokuWiki and still other ODT files.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>autotests/data/redundantDataDescriptorsNoSignature.zip <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/redundantDataDescriptorsWithSignature.zip <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/karchivetest.h <span style="color: grey">(8c4f980)</span></li>
<li>autotests/karchivetest.cpp <span style="color: grey">(4dc016e)</span></li>
<li>src/kzip.cpp <span style="color: grey">(fd9a5e0)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122579/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>