On March 13, 2014, 5:30 p.m., David Jarvie wrote:
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 0x instead of
0x (= -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
value 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.
Dawit Alemayehu wrote:
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.
Ok, I misinterpreted the description. The KDateTime aspects of the patch look
ok apart from one issue mentioned in my detailed comment below.
- David
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116784/#review52897
---
On March 13, 2014, 12:49 p.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116784/
---
(Updated March 13, 2014, 12:49 p.m.)
Review request for kdelibs, Andreas Hartmetz and David Faure.
Repository: kdelibs
Description
---
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.
Diffs
-
kioslave/http/http.h dd85622
kioslave/http/http.cpp e4f1eba
Diff: https://git.reviewboard.kde.org/r/116784/diff/
Testing
---
Thanks,
Dawit Alemayehu
