Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

select: make Curl_socket_check take timediff_t timeout #5240

Closed
wants to merge 2 commits into from

Conversation

@bagder
Copy link
Member

bagder commented Apr 15, 2020

Coverity found CID 1461718:

Integer handling issues (CONSTANT_EXPRESSION_RESULT) "timeout_ms >
9223372036854775807L" is always false regardless of the values of its
operands. This occurs as the logical second operand of "||".

@mback2k
Copy link
Member

mback2k commented Apr 15, 2020

The same condition check would need to be applied to socks.c and schannel.c.

And I am preparing a PR to make the functions in select.c take timediff_t instead of time_t and handle the conversion internally. What do you think about this?

@bagder
Copy link
Member Author

bagder commented Apr 15, 2020

Sounds like a good move! I'll pause this PR then and we can close it again once your work lands.

@mback2k mback2k self-assigned this Apr 15, 2020
@bagder
Copy link
Member Author

bagder commented Apr 15, 2020

Then I will also hold off a fix for two if(timeout_ms < 0) checks in select.c where timeout_ms is a time_t. Those are bugs because we allow time_t to be unsigned on platforms...

@jay
Copy link
Member

jay commented Apr 15, 2020

I think we should ignore the analyzer in this case. Any compiler can optimize it away as is.

@bagder
Copy link
Member Author

bagder commented Apr 15, 2020

I agree that it is a totally harmless detail, but I think this code also risk causing compiler warnings so I still think we should take care of it.

mback2k added a commit to mback2k/curl that referenced this pull request Apr 20, 2020
Make all functions in select.[ch] take timeout_ms as timediff_t
which should always be large enough and signed on all platforms
to take all possible timeout values and avoid type conversions.

Related to curl#5240 and curl#5262
Replaces curl#5107
mback2k added a commit to mback2k/curl that referenced this pull request Apr 20, 2020
Now that all functions in select.[ch] take timediff_t instead
of the limited time_t or int, we can remove type conversions
and related preprocessor checks to silence compiler warnings.

Related to curl#5262
Supersedes curl#5214, curl#5220 and curl#5221
Closes curl#5240
mback2k added a commit to mback2k/curl that referenced this pull request Apr 20, 2020
Make all functions in select.[ch] take timeout_ms as timediff_t
which should always be large enough and signed on all platforms
to take all possible timeout values and avoid type conversions.

Related to curl#5240 and curl#5262
Replaces curl#5107
mback2k added a commit to mback2k/curl that referenced this pull request Apr 20, 2020
Now that all functions in select.[ch] take timediff_t instead
of the limited time_t or int, we can remove type conversions
and related preprocessor checks to silence compiler warnings.

Related to curl#5262
Supersedes curl#5214, curl#5220 and curl#5221
Closes curl#5240
Coverity found CID 1461718:

Integer handling issues (CONSTANT_EXPRESSION_RESULT) "timeout_ms >
9223372036854775807L" is always false regardless of the values of its
operands. This occurs as the logical second operand of "||".
@bagder bagder changed the title gopher: condition the timeout check to satisfy code analyzer select: make Curl_socket_check take timediff_t timeout Apr 23, 2020
@bagder bagder force-pushed the bagder/gopher-timecheck branch from 3186b4c to a78bb56 Apr 23, 2020
@bagder
Copy link
Member Author

bagder commented Apr 23, 2020

I just want to add that this is meant to primarily address the problem Coverity detected before the pending release, it is not a replacement of #5262.

@bagder bagder closed this in a96c752 Apr 23, 2020
@bagder bagder deleted the bagder/gopher-timecheck branch Apr 23, 2020
jay added a commit to jay/curl that referenced this pull request Apr 23, 2020
Follow-up to a96c752 which changed the timeout_ms type from time_t to
timediff_t.

Ref: curl#5240

Closes #xxxx
jay added a commit that referenced this pull request May 2, 2020
Follow-up to a96c752 which changed the timeout_ms type from time_t to
timediff_t.

Ref: #5240

Closes #5286
mback2k added a commit to mback2k/curl that referenced this pull request May 5, 2020
Make all functions in select.[ch] take timeout_ms as timediff_t
which should always be large enough and signed on all platforms
to take all possible timeout values and avoid type conversions.

Related to curl#5240 and curl#5286
Replaces curl#5107 and partially curl#5262
mback2k added a commit to mback2k/curl that referenced this pull request May 6, 2020
Make all functions in select.[ch] take timeout_ms as timediff_t
which should always be large enough and signed on all platforms
to take all possible timeout values and avoid type conversions.

Replaces curl#5107 and partially curl#5262
Related to curl#5240 and curl#5286
Closes curl#5343
mback2k added a commit to mback2k/curl that referenced this pull request May 26, 2020
Make all functions in select.[ch] take timeout_ms as timediff_t
which should always be large enough and signed on all platforms
to take all possible timeout values and avoid type conversions.

Replaces curl#5107 and partially curl#5262
Related to curl#5240 and curl#5286
Closes curl#5343
mback2k added a commit that referenced this pull request May 30, 2020
Make all functions in select.[ch] take timeout_ms as timediff_t
which should always be large enough and signed on all platforms
to take all possible timeout values and avoid type conversions.

Reviewed-by: Jay Satiro
Reviewed-by: Daniel Stenberg

Replaces #5107 and partially #5262
Related to #5240 and #5286
Closes #5343
mback2k added a commit to mback2k/curl that referenced this pull request May 30, 2020
Now that all functions in select.[ch] take timediff_t instead
of the limited time_t or int, we can remove type conversions
and related preprocessor checks to silence compiler warnings.

Related to curl#5262
Supersedes curl#5214, curl#5220 and curl#5221
Closes curl#5240
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.