Re: Review Request 116784: Fix incorrect use of KDateTime.toTime_t in kio_http
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116784/ --- (Updated March 20, 2014, 1:10 p.m.) Status -- This change has been marked as submitted. 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
Re: Review Request 116784: Fix incorrect use of KDateTime.toTime_t in kio_http
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
Re: Review Request 116784: Fix incorrect use of KDateTime.toTime_t in kio_http
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116784/#review52940 --- kioslave/http/http.cpp https://git.reviewboard.kde.org/r/116784/#comment37258 Need to convert to UTC first, since it will produce the wrong value if it's in some random time zone: dt.toUtc().dateTime() - David Jarvie 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
Re: Review Request 116784: Fix incorrect use of KDateTime.toTime_t in kio_http
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116784/#review52897 --- 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. - David Jarvie 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
Re: Review Request 116784: Fix incorrect use of KDateTime.toTime_t in kio_http
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. 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
