CDRIVER-4781 Avoid intermediate uint32_t for socketTimeoutMS#1475
Merged
eramongodb merged 10 commits intomongodb:masterfrom Nov 13, 2023
Merged
CDRIVER-4781 Avoid intermediate uint32_t for socketTimeoutMS#1475eramongodb merged 10 commits intomongodb:masterfrom
eramongodb merged 10 commits intomongodb:masterfrom
Conversation
eramongodb
commented
Nov 13, 2023
kevinAlbs
approved these changes
Nov 13, 2023
Collaborator
kevinAlbs
left a comment
There was a problem hiding this comment.
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
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
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_ttimeout parameters in internal read/write utility functions, where due to historical reasons,int64_ttimeout parameters for several read/write operations are forwarded to underlyingint32_ttimeout parameters (narrowing conversion). The new range checks are (correctly but unexpectedly) triggered bymongoc_cluster_t::sockettimeoutmswhen 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-1when cast back toint32_t:This PR restores support for negative values for the
socketTimeoutMSURI option for backward compatibility by replacing the intermediateuint32_tdata members ofmongoc_cluster_twithint32_tfor 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(PRId64forint64_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.