-
Notifications
You must be signed in to change notification settings - Fork 455
CDRIVER-4781 Avoid intermediate uint32_t for socketTimeoutMS #1475
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eramongodb
commented
Nov 13, 2023
kevinAlbs
approved these changes
Nov 13, 2023
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.
LGTM with minor suggestions.
eramongodb
added a commit
that referenced
this pull request
Nov 14, 2023
* Fix format specifier in timeout range validation error messages * CDRIVER-4781 Avoid overflow of -1 sockettimeout in cast to intermediate uint32_t * Add regression test for negative sockettimeoutms validation * Update documentation with warnings for non-positive timeout values * Update timeout variables for type consistency * Add regression test for unspecified default timeout in writev functions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves CDRIVER-4781. Verified by this patch.
#1229 introduced range representability checks for conversion from
int64_t
->int32_t
timeout parameters in internal read/write utility functions, where due to historical reasons,int64_t
timeout parameters for several read/write operations are forwarded to underlyingint32_t
timeout parameters (narrowing conversion). The new range checks are (correctly but unexpectedly) triggered bymongoc_cluster_t::sockettimeoutms
when the URI option is set to-1
, which has also been (implicitly) supported for historical reasons. The negative value was being implicitly converted to a large unsigned value before being passed as anint64_t
, before being converted back to-1
when cast back toint32_t
:This PR restores support for negative values for the
socketTimeoutMS
URI option for backward compatibility by replacing the intermediateuint32_t
data members ofmongoc_cluster_t
withint32_t
for better consistency with how the values are both assigned and used internally. This avoids the implicit conversion into a large unsigned value that triggers the range check before conversion back to anint32_t
. This change also addresses 5 GCC and 5 Clang warnings for implicit sign conversion. Other instances of internal representation of timeout values (that aren't ignored, i.e. gridfs functions) do not seem to be affected: there are no other instances of intermediately-representing the timeout as an unsigned integer.A regression test is added that ensures the range check is not triggered by a negative socketTimeoutMS URI value. This regression test + git bisect verified that the breaking change was triggered by abb0bb7 (#1229).
As a drive-by fix, this PR also fixes the incorrect format specifier for error messages concerning
timeout_ms
(PRId64
forint64_t
).Additionally, an attempt is made to document the unspecified/inconsistent behavior of non-positive timeout values for the URI option documentation and for functions that have a timeout parameter (i.e.
mongoc_stream_*()
). Scanning through how the timeout values are both represented and interpreted within libmongoc, it does not seem desirable to continue to support non-positive URI timeout values (even0
) despite historical (implicit) support.