<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/116784/">https://git.reviewboard.kde.org/r/116784/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 13th, 2014, 5:30 p.m. UTC, <b>David Jarvie</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;">The handling of return values from KDateTime::toTime_t() in the existing kio_http code is not correct, because the return value's type is implicitly cast to other types before being checked. For example, in one place it is cast to qint64, which will result in a value of 0xffffffff instead of 0xffffffffffffffff (= -1). This type of error will mask the fact that the error value is being returned. Instead of changing the calling code to detect invalid dates using other methods, it should be fixed to properly cast the uint value returned from KDateTime::toTime_t(). For types other than int, it needs to specifically check for uint(-1) and set the cast va
lue to -1 in that case. For example:
uint t = KDateTime::toTime_t(...);
// Set the qint64 to be -1 if an error occurred:
qint64 result = (t == uint(-1)) ? -1 : t;
Note: KDateTime::toTime_t() is *supposed* to return uint(-1) to indicate an error. If it doesn't always do this, *it* should be fixed instead of changing code elsewhere, since kio_http is unlikely to be the only module that will have trouble if that is happening.</pre>
</blockquote>
<p>On March 13th, 2014, 11:27 p.m. UTC, <b>Dawit Alemayehu</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;">Perhaps it was not clear from the description, but I am not implying nor have I implied there to be a bug in KDateTime. As I have clearly stated the problem is with the assumption the code in kio_http makes about what KDateTime::toTime_t returns for an invalid date. No matter how you see it the toTime_t() function can not and does not return a literal -1, which is exactly what the code in kio_http assumes! Of course that is clearly wrong. Anyhow, this patch is specifically intended to fix that issue and nothing else.</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;">Ok, I misinterpreted the description. The KDateTime aspects of the patch look ok apart from one issue mentioned in my detailed comment below.</pre>
<br />
<p>- David</p>
<br />
<p>On March 13th, 2014, 12:49 p.m. UTC, Dawit Alemayehu wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs, Andreas Hartmetz and David Faure.</div>
<div>By Dawit Alemayehu.</div>
<p style="color: grey;"><i>Updated March 13, 2014, 12:49 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</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;">The attached patch does the following:
- It corrects a mistake in assumption that KDateTime.toTime_t() will return -1 for invalidate dates. It does not. The result is an overflow which is interpreted in kio_http as a timestamp in the distant future which obviously is wrong. See https://bugs.kde.org/show_bug.cgi?id=331774 for example. This assumption also affects the timestamp variables used for cache management.
- It converts cache management timestamp variables to 64 bits so they can accomodates dates beyond Feb 7, 2106.</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>kioslave/http/http.h <span style="color: grey">(dd85622)</span></li>
<li>kioslave/http/http.cpp <span style="color: grey">(e4f1eba)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/116784/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>