Y2038: Use time_t for commSetConnTimeout() timeout parameter #1492
Conversation
Change the "timeout" parameter to commSetConnTimeout from int to time_t, to ensure 2032-safety on systems where int is 32-bit. Detected by Coverity scan, CID 1545129 "Use of 32-bit time_t"
There was a problem hiding this comment.
Thank you for improving this code.
We are using negative commSetConnTimeout() timeout values while time_t is not, technically, guaranteed to be signed. However, this PR can still be considered a step forward even though it does not solve that (unlikely to affect us in real world) problem because nearly all commSetConnTimeout() callers do use time_t already.
rousskov
changed the title
|
I adjusted PR title/description, primarily to fix a few minor technical issues and typos. Please adjust further as needed. |
rousskov
added
the
S-waiting-for-author
Apply review suggestion Co-authored-by: Alex Rousskov <[email protected]>
kinkie
added
S-waiting-for-reviewer
Co-authored-by: Alex Rousskov <[email protected]>
rousskov
added
M-cleared-for-merge
|
I adjusted PR description again to use the original/shorter version of the Y2K38_SAFETY defect wording. I did not have access to that version earlier because Coverity service was down. Sorry. |
|
RFC: long-term we are better to be moving to the |
Yes, of course.
I think the answer depends, in part, on whether we want to start supporting sub-second timeouts. I cannot be sure without studying this issue, but I suspect that we do. If we do, then, for commSetConnTimeout(), the migration to std::chrono should probably start at the timeout storing/handling code, not its users (that this PR is focusing on) because while it is possible to safely convert time_t to the right std::chrono type, it is not possible to safely convert any sub-second std::chrono type to time_t. FWIW, my "use PackableStream to print time_t" suggestions in #1493 (review) and #1494 (review) are tiny steps towards that std::chrono conversion. |
Change commSetConnTimeout() "timeout" parameter from int to time_t, to match the common caller type and improve Year 2038-safety on systems with 32-bit int. Detected by Coverity. CID 1545129: Use of 32-bit time_t (Y2K38_SAFETY).
squid-anubis
added
the
M-waiting-staging-checks
squid-anubis
added
M-merged
| @@ -592,7 +592,7 @@ commUnsetFdTimeout(int fd) | |||
| } | |||
|
|
|||
| int | |||
| commSetConnTimeout(const Comm::ConnectionPointer &conn, int timeout, AsyncCall::Pointer &callback) | |||
| commSetConnTimeout(const Comm::ConnectionPointer &conn, time_t timeout, AsyncCall::Pointer &callback) | |||
There was a problem hiding this comment.
I did not realize this by looking at the PR diff, but I now see two more problems related to this (merged) PR scope:
- This function and commUnsetConnTimeout() return type should be
time_trather thanint. - The C-style
timeoutcast inside this function should be removed.
@kinkie, if you can fix these problems, please do.
The latest Coverity report (defect 1547031) triggered this comment.
Change commSetConnTimeout() "timeout" parameter from int to time_t, to
match the common caller type and improve Year 2038-safety on systems
with 32-bit int.
Detected by Coverity. CID 1545129: Use of 32-bit time_t (Y2K38_SAFETY).