Skip to content

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
merged 10 commits into from
Nov 13, 2023

Conversation

eramongodb
Copy link
Contributor

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 underlying int32_t timeout parameters (narrowing conversion). The new range checks are (correctly but unexpectedly) triggered by mongoc_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 an int64_t, before being converted back to -1 when cast back to int32_t:

-1         (int32_t: MONGOC_URI_SOCKETTIMEOUTMS)
4294967295 (uint32_t: mongoc_cluster_t::sockettimeoutms)
4294967295 (int64_t: _mongoc_stream_writev_full())
  >int32_t range check failure< (the breaking change)
-1         (int32_t: mongoc_stream_writev(), impl-defined narrowing signed conversion)

This PR restores support for negative values for the socketTimeoutMS URI option for backward compatibility by replacing the intermediate uint32_t data members of mongoc_cluster_t with int32_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 an int32_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 for int64_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 (even 0) despite historical (implicit) support.

@eramongodb eramongodb requested a review from kevinAlbs November 13, 2023 17:27
@eramongodb eramongodb self-assigned this Nov 13, 2023
Copy link
Collaborator

@kevinAlbs kevinAlbs left a 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 eramongodb merged commit 42ff072 into mongodb:master Nov 13, 2023
@eramongodb eramongodb deleted the cdriver-4781 branch November 13, 2023 21:42
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants