CDRIVER-4781 Avoid intermediate uint32_t for socketTimeoutMS (#1475)

eramongodb · eramongodb · commit 6cef90acb2e2 · 2023-11-14T09:23:47.000-06:00
* 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
diff --git a/src/libmongoc/doc/mongoc_stream_read.rst b/src/libmongoc/doc/mongoc_stream_read.rst
@@ -26,6 +26,13 @@ Parameters
 
 The :symbol:`mongoc_stream_read()` function shall perform a read from a :symbol:`mongoc_stream_t`. It's modeled on the API and semantics of ``read()``, though the parameters map only loosely.
 
+.. warning::
+
+  The "default timeout" indicated by a negative value is both unspecified and
+  unrelated to the documented default values for ``*TimeoutMS`` URI options.
+  To specify a default timeout value for a ``*TimeoutMS`` URI option, use the
+  ``MONGOC_DEFAULT_*`` constants defined in ``mongoc-client.h``.
+
 Returns
 -------
 
@@ -38,4 +45,3 @@ The :symbol:`mongoc_stream_read` function returns the number of bytes read on su
   | :symbol:`mongoc_stream_write()`
 
   | :symbol:`mongoc_stream_writev()`
-
diff --git a/src/libmongoc/doc/mongoc_stream_readv.rst b/src/libmongoc/doc/mongoc_stream_readv.rst
@@ -26,6 +26,13 @@ Parameters
 
 This function is identical to :symbol:`mongoc_stream_read()` except that it takes a :symbol:`mongoc_iovec_t` to perform gathered I/O.
 
+.. warning::
+
+  The "default timeout" indicated by a negative value is both unspecified and
+  unrelated to the documented default values for ``*TimeoutMS`` URI options.
+  To specify a default timeout value for a ``*TimeoutMS`` URI option, use the
+  ``MONGOC_DEFAULT_*`` constants defined in ``mongoc-client.h``.
+
 Returns
 -------
 
@@ -38,4 +45,3 @@ Returns
   | :symbol:`mongoc_stream_write()`
 
   | :symbol:`mongoc_stream_writev()`
-
diff --git a/src/libmongoc/doc/mongoc_stream_write.rst b/src/libmongoc/doc/mongoc_stream_write.rst
@@ -24,6 +24,13 @@ Parameters
 
 The :symbol:`mongoc_stream_write()` function shall perform a write to a :symbol:`mongoc_stream_t`. It's modeled on the API and semantics of ``write()``, though the parameters map only loosely.
 
+.. warning::
+
+  The "default timeout" indicated by a negative value is both unspecified and
+  unrelated to the documented default values for ``*TimeoutMS`` URI options.
+  To specify a default timeout value for a ``*TimeoutMS`` URI option, use the
+  ``MONGOC_DEFAULT_*`` constants defined in ``mongoc-client.h``.
+
 Returns
 -------
 
@@ -36,4 +43,3 @@ The :symbol:`mongoc_stream_write` function returns the number of bytes written o
   | :symbol:`mongoc_stream_readv()`
 
   | :symbol:`mongoc_stream_writev()`
-
diff --git a/src/libmongoc/doc/mongoc_stream_writev.rst b/src/libmongoc/doc/mongoc_stream_writev.rst
@@ -27,8 +27,14 @@ to a :symbol:`mongoc_stream_t`. It's modeled on the
 API and semantics of ``writev()``, though the parameters map only
 loosely.
 
+.. warning::
+
+  The "default timeout" indicated by a negative value is both unspecified and
+  unrelated to the documented default values for ``*TimeoutMS`` URI options.
+  To specify a default timeout value for a ``*TimeoutMS`` URI option, use the
+  ``MONGOC_DEFAULT_*`` constants defined in ``mongoc-client.h``.
+
 Returns
 -------
 
 The number of bytes written on success, or ``-1`` upon failure and ``errno`` is set.
-
diff --git a/src/libmongoc/doc/mongoc_uri_t.rst b/src/libmongoc/doc/mongoc_uri_t.rst
@@ -104,7 +104,12 @@ MONGOC_URI_LOADBALANCED                    loadbalanced                      fal
 MONGOC_URI_SRVMAXHOSTS                     srvmaxhosts                       0                                 If zero, the number of hosts in DNS results is unlimited. If greater than zero, the number of hosts in DNS results is limited to being less than or equal to the given value.
 ========================================== ================================= ================================= ============================================================================================================================================================================================================================================
 
-Setting any of the \*timeoutMS options above to ``0`` will be interpreted as "use the default value".
+.. warning::
+
+  Setting any of the \*timeoutMS options above to either ``0`` or a negative value is discouraged due to unspecified and inconsistent behavior.
+  The "default value" historically specified as a fallback for ``0`` or a negative value is NOT related to the default values for the \*timeoutMS options documented above.
+  The meaning of a timeout of ``0`` or a negative value may vary depending on the operation being executed, even when specified by the same URI option.
+  To specify the documented default value for a \*timeoutMS option, use the `MONGOC_DEFAULT_*` constants defined in ``mongoc-client.h`` instead.
 
 Authentication Options
 ----------------------
@@ -330,4 +335,3 @@ MONGOC_URI_SAFE                            safe                              {tr
     mongoc_uri_set_username
     mongoc_uri_set_write_concern
     mongoc_uri_unescape
-
diff --git a/src/libmongoc/doc/mongoc_write_concern_set_wtimeout.rst b/src/libmongoc/doc/mongoc_write_concern_set_wtimeout.rst
@@ -31,4 +31,3 @@ instead of altering the write concern.
 .. seealso::
 
   | :symbol:`mongoc_write_concern_get_wtimeout` and :symbol:`mongoc_write_concern_set_wtimeout_int64`.
-
diff --git a/src/libmongoc/src/mongoc/mongoc-buffer.c b/src/libmongoc/src/mongoc/mongoc-buffer.c
@@ -199,7 +199,7 @@ _mongoc_buffer_append_from_stream (mongoc_buffer_t *buffer,
       bson_set_error (error,
                       MONGOC_ERROR_STREAM,
                       MONGOC_ERROR_STREAM_SOCKET,
-                      "timeout_msec value %" PRIu64
+                      "timeout_msec value %" PRId64
                       " exceeds supported 32-bit range",
                       timeout_msec);
       RETURN (false);
@@ -266,7 +266,7 @@ _mongoc_buffer_fill (mongoc_buffer_t *buffer,
       bson_set_error (error,
                       MONGOC_ERROR_STREAM,
                       MONGOC_ERROR_STREAM_SOCKET,
-                      "timeout_msec value %" PRIu64
+                      "timeout_msec value %" PRId64
                       " exceeds supported 32-bit range",
                       timeout_msec);
       RETURN (false);
@@ -342,7 +342,7 @@ _mongoc_buffer_try_append_from_stream (mongoc_buffer_t *buffer,
 
    if (BSON_UNLIKELY (!bson_in_range_signed (int32_t, timeout_msec))) {
       // CDRIVER-4589
-      MONGOC_ERROR ("timeout_msec value %" PRIu64
+      MONGOC_ERROR ("timeout_msec value %" PRId64
                     " exceeds supported 32-bit range",
                     timeout_msec);
       RETURN (-1);
diff --git a/src/libmongoc/src/mongoc/mongoc-cluster-private.h b/src/libmongoc/src/mongoc/mongoc-cluster-private.h
@@ -53,8 +53,8 @@ typedef struct _mongoc_cluster_node_t {
 typedef struct _mongoc_cluster_t {
    int64_t operation_id;
    int32_t request_id;
-   uint32_t sockettimeoutms;
-   uint32_t socketcheckintervalms;
+   int32_t sockettimeoutms;
+   int32_t socketcheckintervalms;
    mongoc_uri_t *uri;
    unsigned requires_auth : 1;
 
diff --git a/src/libmongoc/src/mongoc/mongoc-crypt.c b/src/libmongoc/src/mongoc/mongoc-crypt.c
@@ -491,9 +491,7 @@ _state_need_kms (_state_machine_t *state_machine, bson_error_t *error)
    mongocrypt_binary_t *http_req = NULL;
    mongocrypt_binary_t *http_reply = NULL;
    const char *endpoint;
-   uint32_t sockettimeout;
-
-   sockettimeout = MONGOC_DEFAULT_SOCKETTIMEOUTMS;
+   const int32_t sockettimeout = MONGOC_DEFAULT_SOCKETTIMEOUTMS;
    kms_ctx = mongocrypt_ctx_next_kms_ctx (state_machine->ctx);
    while (kms_ctx) {
       mongoc_iovec_t iov;
diff --git a/src/libmongoc/src/mongoc/mongoc-secure-channel.c b/src/libmongoc/src/mongoc/mongoc-secure-channel.c
@@ -476,7 +476,7 @@ mongoc_secure_channel_read (mongoc_stream_tls_t *tls,
 
    if (BSON_UNLIKELY (!bson_in_range_signed (int32_t, tls->timeout_msec))) {
       // CDRIVER-4589
-      MONGOC_ERROR ("timeout_msec value %" PRIu64
+      MONGOC_ERROR ("timeout_msec value %" PRId64
                     " exceeds supported 32-bit range",
                     tls->timeout_msec);
       return -1;
@@ -511,7 +511,7 @@ mongoc_secure_channel_write (mongoc_stream_tls_t *tls,
 
    if (BSON_UNLIKELY (!bson_in_range_signed (int32_t, tls->timeout_msec))) {
       // CDRIVER-4589
-      MONGOC_ERROR ("timeout_msec value %" PRIu64
+      MONGOC_ERROR ("timeout_msec value %" PRId64
                     " exceeds supported 32-bit range",
                     tls->timeout_msec);
       return -1;
diff --git a/src/libmongoc/src/mongoc/mongoc-stream-tls-openssl-bio.c b/src/libmongoc/src/mongoc/mongoc-stream-tls-openssl-bio.c
@@ -218,7 +218,7 @@ mongoc_stream_tls_openssl_bio_read (BIO *b, char *buf, int len)
 
    if (BSON_UNLIKELY (!bson_in_range_signed (int32_t, tls->timeout_msec))) {
       // CDRIVER-4589
-      MONGOC_ERROR ("timeout_msec value %" PRIu64
+      MONGOC_ERROR ("timeout_msec value %" PRId64
                     " exceeds supported 32-bit range",
                     tls->timeout_msec);
       return -1;
@@ -289,7 +289,7 @@ mongoc_stream_tls_openssl_bio_write (BIO *b, const char *buf, int len)
 
    if (BSON_UNLIKELY (!bson_in_range_signed (int32_t, tls->timeout_msec))) {
       // CDRIVER-4589
-      MONGOC_ERROR ("timeout_msec value %" PRIu64
+      MONGOC_ERROR ("timeout_msec value %" PRId64
                     " exceeds supported 32-bit range",
                     tls->timeout_msec);
       RETURN (-1);
diff --git a/src/libmongoc/src/mongoc/mongoc-stream.c b/src/libmongoc/src/mongoc/mongoc-stream.c
@@ -157,6 +157,7 @@ mongoc_stream_writev (mongoc_stream_t *stream,
 
    BSON_ASSERT (stream->writev);
 
+   // CDRIVER-4781: for backward compatibility.
    if (timeout_msec < 0) {
       timeout_msec = MONGOC_DEFAULT_TIMEOUT_MSEC;
    }
@@ -441,7 +442,7 @@ _mongoc_stream_writev_full (mongoc_stream_t *stream,
       bson_set_error (error,
                       MONGOC_ERROR_STREAM,
                       MONGOC_ERROR_STREAM_SOCKET,
-                      "timeout_msec value %" PRIu64
+                      "timeout_msec value %" PRId64
                       " exceeds supported 32-bit range",
                       timeout_msec);
       RETURN (false);
diff --git a/src/libmongoc/tests/test-mongoc-cluster.c b/src/libmongoc/tests/test-mongoc-cluster.c
@@ -752,6 +752,32 @@ test_cluster_time_insert_pooled (void)
 }
 
 
+static void
+test_cluster_command_timeout_negative (void)
+{
+   bson_error_t error;
+
+   mongoc_uri_t *const uri = test_framework_get_uri ();
+
+   // CDRIVER-4781: libmongoc historically supports negative values as
+   // fallback to a "default" value for timeouts.
+   mongoc_uri_set_option_as_int32 (uri, MONGOC_URI_SOCKETTIMEOUTMS, -1);
+
+   mongoc_client_t *const client =
+      test_framework_client_new_from_uri (uri, NULL);
+   test_framework_set_ssl_opts (client);
+
+   // There should not be an error when validating sockettimeoutms.
+   ASSERT_OR_PRINT (
+      mongoc_client_command_simple (
+         client, "admin", tmp_bson ("{'ping': 1}"), NULL, NULL, &error),
+      error);
+
+   mongoc_client_destroy (client);
+   mongoc_uri_destroy (uri);
+}
+
+
 static void
 replies_with_cluster_time (request_t *request,
                            int t,
@@ -1924,6 +1950,9 @@ test_cluster_install (TestSuite *suite)
    TestSuite_AddLive (suite,
                       "/Cluster/cluster_time/insert/pooled",
                       test_cluster_time_insert_pooled);
+   TestSuite_AddLive (suite,
+                      "/Cluster/command/timeout/negative",
+                      test_cluster_command_timeout_negative);
    TestSuite_AddMockServerTest (suite,
                                 "/Cluster/cluster_time/comparison/single",
                                 test_cluster_time_comparison_single,
diff --git a/src/libmongoc/tests/test-mongoc-stream.c b/src/libmongoc/tests/test-mongoc-stream.c
@@ -164,11 +164,126 @@ test_stream_writev_full (void)
    mongoc_stream_destroy (success_stream);
 }
 
+typedef struct {
+   mongoc_stream_t vtable;
+   int32_t timeout_msec;
+   bool is_set;
+} writev_timeout_stream_t;
+
+static void
+_writev_timeout_stream_destroy (mongoc_stream_t *stream)
+{
+   bson_free (stream);
+}
+
+static ssize_t
+_writev_timeout_stream_writev (mongoc_stream_t *stream_param,
+                               mongoc_iovec_t *iov,
+                               size_t iovcnt,
+                               int32_t timeout_msec)
+{
+   BSON_UNUSED (iov);
+   BSON_UNUSED (iovcnt);
+
+   writev_timeout_stream_t *const stream =
+      (writev_timeout_stream_t *) stream_param;
+
+   stream->is_set = true;
+   stream->timeout_msec = timeout_msec;
+
+   return 0;
+}
+
+static writev_timeout_stream_t *
+_writev_timeout_stream_new (void)
+{
+   writev_timeout_stream_t *const stream =
+      bson_malloc (sizeof (writev_timeout_stream_t));
+
+   *stream = (writev_timeout_stream_t){
+      .vtable =
+         {
+            .type = 999, // For testing purposes.
+            .destroy = _writev_timeout_stream_destroy,
+            .writev = _writev_timeout_stream_writev,
+         },
+      .is_set = false,
+      .timeout_msec = 0,
+   };
+
+   return stream;
+}
+
+static void
+test_stream_writev_timeout (void)
+{
+   bson_error_t error;
+
+   uint8_t data[1] = {0};
+   mongoc_iovec_t iov = {.iov_base = data, .iov_len = 1u};
+
+   // A positive timeout value should be forwarded as-is to the writev function.
+   {
+      writev_timeout_stream_t *const stream = _writev_timeout_stream_new ();
+
+      ssize_t const res =
+         _mongoc_stream_writev_full ((mongoc_stream_t *) stream,
+                                     &iov,
+                                     1u,
+                                     MONGOC_DEFAULT_SOCKETTIMEOUTMS,
+                                     &error);
+      ASSERT_CMPSSIZE_T (res, ==, 0);
+      ASSERT_WITH_MSG (
+         stream->is_set,
+         "expected _writev_timeout_stream_writev() to be invoked");
+      ASSERT_CMPINT32 (
+         stream->timeout_msec, ==, MONGOC_DEFAULT_SOCKETTIMEOUTMS);
+
+      mongoc_stream_destroy ((mongoc_stream_t *) stream);
+   }
+
+   // A timeout value of 0 should be forwarded as-is to the writev function.
+   {
+      writev_timeout_stream_t *const stream = _writev_timeout_stream_new ();
+
+      ssize_t const res = _mongoc_stream_writev_full (
+         (mongoc_stream_t *) stream, &iov, 1u, 0, &error);
+      ASSERT_CMPSSIZE_T (res, ==, 0);
+      ASSERT_WITH_MSG (
+         stream->is_set,
+         "expected _writev_timeout_stream_writev() to be invoked");
+      ASSERT_CMPINT32 (stream->timeout_msec, ==, 0);
+
+      mongoc_stream_destroy ((mongoc_stream_t *) stream);
+   }
+
+   // CDRIVER-4781: a negative timeout value will fallback to an unspecified
+   // default value. The writev function should receive the unspecified default
+   // timeout value rather than the negative timeout value.
+   {
+      // See: MONGOC_DEFAULT_TIMEOUT_MSEC in mongoc-stream.c.
+      const int32_t default_timeout_msec = 60 * 60 * 1000;
+
+      writev_timeout_stream_t *const stream = _writev_timeout_stream_new ();
+
+      ssize_t const res = _mongoc_stream_writev_full (
+         (mongoc_stream_t *) stream, &iov, 1u, -1, &error);
+      ASSERT_CMPSSIZE_T (res, ==, 0);
+      ASSERT_WITH_MSG (
+         stream->is_set,
+         "expected _writev_timeout_stream_writev() to be invoked");
+      ASSERT_CMPINT32 (stream->timeout_msec, ==, default_timeout_msec);
+
+      mongoc_stream_destroy ((mongoc_stream_t *) stream);
+   }
+}
+
 
 void
 test_stream_install (TestSuite *suite)
 {
    TestSuite_Add (suite, "/Stream/buffered/basic", test_buffered_basic);
    TestSuite_Add (suite, "/Stream/buffered/oversized", test_buffered_oversized);
-   TestSuite_Add (suite, "/Stream/writev_full", test_stream_writev_full);
+   TestSuite_Add (suite, "/Stream/writev/full", test_stream_writev_full);
+   TestSuite_Add (suite, "/Stream/writev/timeout", test_stream_writev_timeout);
 }

Original file line number	Diff line number	Diff line change
`@@ -31,4 +31,3 @@ instead of altering the write concern.`
`31`	`31`	`.. seealso::`
`32`	`32`
`33`	`33`	\| :symbol:`mongoc_write_concern_get_wtimeout` and :symbol:`mongoc_write_concern_set_wtimeout_int64`.
`34`		`-`