> 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 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 > > 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.
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. - Dawit ----------------------------------------------------------- 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 > >
