-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
select: fix overflow protection #5286
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
Conversation
#if INT_MAX > TIME_T_MAX | ||
if(timeout_ms > (int)TIME_T_MAX) | ||
timeout_ms = (int)TIME_T_MAX; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you mention a platform where this change is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of any platform where INT_MAX > TIME_T_MAX. It's correct to check it though. typecast is because time_t may be unsigned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it might be correct to check, curl doesn't build on any platform where int
is not 32 bit (and time_t
is never smaller than 32 bits either). I'm sure such a theoretical platform would break the curl build in numerous other places. And @mback2k has a larger take on this code in progress anyway. But I won't object if you insist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I can't think of one. For posterity. #5262 may cover this PR, and if not it can be incorporated. I'll wait to see what happens there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jay I would be fine with merging this PR before the release, as my changes will be part of the next feature window.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I will put this in for now and then you can replace it when you're ready.
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
Follow-up to a96c752 which changed the timeout_ms type from time_t to
timediff_t.
Ref: #5240
Closes #xxxx
5240 added a change after the review to typecast timeout_ms to time_t but the problem with that is if time_t max is less than timediff_t max then possible signed overflow