CDRIVER-5864 Do not parse BSON in mongoc_server_description_new_copy (#1835)

* do not parse BSON in `mongoc_server_description_new_copy`

* remove unnecessary atomic modification of `round_trip_time_msec`

* add tests for copying

Author: kevinAlbs

src/libmongoc/src/mongoc/mongoc-server-description.c

@@ -485,11 +485,9 @@ mongoc_server_description_update_rtt (mongoc_server_description_t *server, int64
       return;
    }
    if (server->round_trip_time_msec == MONGOC_RTT_UNSET) {
-      mcommon_atomic_int64_exchange (&server->round_trip_time_msec, rtt_msec, mcommon_memory_order_relaxed);
+      server->round_trip_time_msec = rtt_msec;
    } else {
-      mcommon_atomic_int64_exchange (&server->round_trip_time_msec,
-                                     (int64_t) (ALPHA * rtt_msec + (1 - ALPHA) * server->round_trip_time_msec),
-                                     mcommon_memory_order_relaxed);
+      server->round_trip_time_msec = (int64_t) (ALPHA * rtt_msec + (1 - ALPHA) * server->round_trip_time_msec);
    }
 }
 
@@ -768,48 +766,96 @@ mongoc_server_description_handle_hello (mongoc_server_description_t *sd,
 mongoc_server_description_t *
 mongoc_server_description_new_copy (const mongoc_server_description_t *description)
 {
-   mongoc_server_description_t *copy;
+#define COPY_FIELD(FIELD)               \
+   if (1) {                             \
+      copy->FIELD = description->FIELD; \
+   } else                               \
+      (void) 0
+
+
+#define COPY_BSON_FIELD(FIELD)                          \
+   if (1) {                                             \
+      bson_copy_to (&description->FIELD, &copy->FIELD); \
+   } else                                               \
+      (void) 0
+
+// COPY_INTERNAL_BSON_FIELD copies a `bson_t` that references data in `last_hello_response`.
+#define COPY_INTERNAL_BSON_FIELD(FIELD)                                                                              \
+   if (1) {                                                                                                          \
+      if (!bson_empty (&description->FIELD)) {                                                                       \
+         ptrdiff_t offset = bson_get_data (&description->FIELD) - bson_get_data (&description->last_hello_response); \
+         MONGOC_DEBUG_ASSERT (offset >= 0);                                                                          \
+         const uint8_t *data = bson_get_data (&copy->last_hello_response) + offset;                                  \
+         uint32_t len = description->FIELD.len;                                                                      \
+         MONGOC_DEBUG_ASSERT (offset + len <= copy->last_hello_response.len);                                        \
+         bson_init_static (&copy->FIELD, data, len);                                                                 \
+      } else {                                                                                                       \
+         bson_init (&copy->FIELD);                                                                                   \
+      }                                                                                                              \
+   } else                                                                                                            \
+      (void) 0
+
+// COPY_INTERNAL_STRING_FIELD copies a `const char*` that references data in `last_hello_response`.
+#define COPY_INTERNAL_STRING_FIELD(FIELD)                                                                             \
+   if (1) {                                                                                                           \
+      if (description->FIELD) {                                                                                       \
+         ptrdiff_t offset = (char *) description->FIELD - (char *) bson_get_data (&description->last_hello_response); \
+         MONGOC_DEBUG_ASSERT (offset >= 0);                                                                           \
+         copy->FIELD = (char *) bson_get_data (&copy->last_hello_response) + offset;                                  \
+         MONGOC_DEBUG_ASSERT (offset + strlen (description->FIELD) <= copy->last_hello_response.len);                 \
+      } else {                                                                                                        \
+         copy->FIELD = NULL;                                                                                          \
+      }                                                                                                               \
+   } else                                                                                                             \
+      (void) 0
+
 
    if (!description) {
       return NULL;
    }
 
-   copy = BSON_ALIGNED_ALLOC0 (mongoc_server_description_t);
-
-   copy->id = description->id;
-   copy->opened = description->opened;
-   memcpy (&copy->host, &description->host, sizeof (copy->host));
-   copy->round_trip_time_msec = MONGOC_RTT_UNSET;
+   mongoc_server_description_t *copy = BSON_ALIGNED_ALLOC (mongoc_server_description_t);
 
+   COPY_FIELD (id);
+   COPY_FIELD (host);
+   COPY_FIELD (round_trip_time_msec);
+   COPY_FIELD (last_update_time_usec);
+   COPY_BSON_FIELD (last_hello_response);
+   COPY_FIELD (has_hello_response);
+   COPY_FIELD (hello_ok);
    copy->connection_address = copy->host.host_and_port;
-   bson_init (&copy->last_hello_response);
-   bson_init (&copy->hosts);
-   bson_init (&copy->passives);
-   bson_init (&copy->arbiters);
-   bson_init (&copy->tags);
-   bson_init (&copy->compressors);
-   bson_copy_to (&description->topology_version, &copy->topology_version);
-   bson_oid_copy (&description->service_id, &copy->service_id);
-   copy->server_connection_id = description->server_connection_id;
-
-   if (description->has_hello_response) {
-      /* calls mongoc_server_description_reset */
-      int64_t last_rtt_ms =
-         mcommon_atomic_int64_fetch (&description->round_trip_time_msec, mcommon_memory_order_relaxed);
-      mongoc_server_description_handle_hello (
-         copy, &description->last_hello_response, last_rtt_ms, &description->error);
-   } else {
-      mongoc_server_description_reset (copy);
-      /* preserve the original server description type, which is manually set
-       * for a LoadBalancer server */
-      copy->type = description->type;
-   }
+   COPY_INTERNAL_STRING_FIELD (me);
+   COPY_FIELD (opened);
+   COPY_INTERNAL_STRING_FIELD (set_name);
+   COPY_FIELD (error);
+   COPY_FIELD (type);
+   COPY_FIELD (min_wire_version);
+   COPY_FIELD (max_wire_version);
+   COPY_FIELD (max_msg_size);
+   COPY_FIELD (max_bson_obj_size);
+   COPY_FIELD (max_write_batch_size);
+   COPY_FIELD (session_timeout_minutes);
+   COPY_INTERNAL_BSON_FIELD (hosts);
+   COPY_INTERNAL_BSON_FIELD (passives);
+   COPY_INTERNAL_BSON_FIELD (arbiters);
+   COPY_INTERNAL_BSON_FIELD (tags);
+   COPY_INTERNAL_STRING_FIELD (current_primary);
+   COPY_FIELD (set_version);
+   COPY_FIELD (election_id);
+   COPY_FIELD (last_write_date_ms);
+   COPY_INTERNAL_BSON_FIELD (compressors);
+   // `topology_version` does not refer to data in `last_hello_response`. It needs to outlive `last_hello_response`.
+   COPY_BSON_FIELD (topology_version);
+   COPY_FIELD (generation);
+   copy->_generation_map_ = mongoc_generation_map_copy (mc_tpl_sd_generation_map_const (description));
+   COPY_FIELD (service_id);
+   COPY_FIELD (server_connection_id);
 
-   /* Preserve the error */
-   memcpy (&copy->error, &description->error, sizeof copy->error);
+#undef COPY_INTERNAL_STRING_FIELD
+#undef COPY_INTERNAL_BSON_FIELD
+#undef COPY_BSON_FIELD
+#undef COPY_FIELD
 
-   copy->generation = description->generation;
-   copy->_generation_map_ = mongoc_generation_map_copy (mc_tpl_sd_generation_map_const (description));
    return copy;
 }
 

src/libmongoc/tests/test-mongoc-server-description.c

@@ -458,6 +458,119 @@ test_server_description_hello_type_error (void)
    mongoc_server_description_cleanup (&sd);
 }
 
+static void
+test_copy (const char *hello_json)
+{
+   mongoc_server_description_t sd, *sd_copy;
+   mongoc_server_description_init (&sd, "host:1234", 1);
+   bson_error_t empty_error = {0};
+   mongoc_server_description_handle_hello (&sd, tmp_bson (hello_json), 0, &empty_error);
+   sd_copy = mongoc_server_description_new_copy (&sd);
+
+   // Check server descriptions compare equal by "Server Description Equality" rules. Not all fields are considered.
+   ASSERT (_mongoc_server_description_equal (&sd, sd_copy));
+
+   // Check all fields:
+   ASSERT_CMPUINT32 (sd.id, ==, sd_copy->id);
+   ASSERT_CMPSTR (sd.host.host_and_port, sd_copy->host.host_and_port);
+   ASSERT_CMPINT64 (sd.round_trip_time_msec, ==, sd_copy->round_trip_time_msec);
+   ASSERT_CMPINT64 (sd.last_update_time_usec, ==, sd_copy->last_update_time_usec);
+   ASSERT_EQUAL_BSON (&sd.last_hello_response, &sd_copy->last_hello_response);
+   ASSERT_CMPINT ((int) sd.has_hello_response, ==, (int) sd_copy->has_hello_response);
+   ASSERT_CMPINT ((int) sd.hello_ok, ==, (int) sd_copy->hello_ok);
+   ASSERT_CMPSTR (sd.connection_address, sd_copy->connection_address);
+   ASSERT_CMPSTR (sd.me, sd_copy->me);
+   ASSERT_CMPINT ((int) sd.opened, ==, (int) sd_copy->opened);
+   ASSERT_CMPSTR (sd.set_name, sd_copy->set_name);
+   ASSERT_MEMCMP (&sd.error, &sd_copy->error, (int) sizeof (bson_error_t));
+   ASSERT_CMPINT ((int) sd.type, ==, (int) sd_copy->type);
+   ASSERT_CMPINT32 (sd.min_wire_version, ==, sd_copy->min_wire_version);
+   ASSERT_CMPINT32 (sd.max_wire_version, ==, sd_copy->max_wire_version);
+   ASSERT_CMPINT32 (sd.max_msg_size, ==, sd_copy->max_msg_size);
+   ASSERT_CMPINT32 (sd.max_bson_obj_size, ==, sd_copy->max_bson_obj_size);
+   ASSERT_CMPINT32 (sd.max_write_batch_size, ==, sd_copy->max_write_batch_size);
+   ASSERT_CMPINT64 (sd.session_timeout_minutes, ==, sd_copy->session_timeout_minutes);
+   ASSERT_EQUAL_BSON (&sd.hosts, &sd_copy->hosts);
+   ASSERT_EQUAL_BSON (&sd.passives, &sd_copy->passives);
+   ASSERT_EQUAL_BSON (&sd.arbiters, &sd_copy->arbiters);
+   ASSERT_EQUAL_BSON (&sd.tags, &sd_copy->tags);
+   ASSERT_CMPSTR (sd.current_primary, sd_copy->current_primary);
+   ASSERT_CMPINT64 (sd.set_version, ==, sd_copy->set_version);
+   ASSERT_MEMCMP (&sd.election_id, &sd_copy->election_id, (int) sizeof (bson_oid_t));
+   ASSERT_CMPINT64 (sd.last_write_date_ms, ==, sd_copy->last_write_date_ms);
+   ASSERT_EQUAL_BSON (&sd.compressors, &sd_copy->compressors);
+   ASSERT_EQUAL_BSON (&sd.topology_version, &sd_copy->topology_version);
+   ASSERT_CMPUINT32 (sd.generation, ==, sd_copy->generation);
+   ASSERT (sd_copy->_generation_map_ != NULL); // Do not compare entries. Just ensure non-NULL.
+   ASSERT_MEMCMP (&sd.service_id, &sd_copy->service_id, (int) sizeof (bson_oid_t));
+   ASSERT_CMPINT64 (sd.server_connection_id, ==, sd_copy->server_connection_id);
+
+   mongoc_server_description_cleanup (&sd);
+   mongoc_server_description_destroy (sd_copy);
+}
+
+static void
+test_server_description_copy (void)
+{
+   const char *hello_mongod = BSON_STR ({
+      "topologyVersion" : {"processId" : {"$oid" : "6792ef87965dee8797402adb"}, "counter" : 6},
+      "hosts" : ["localhost:27017"],
+      "setName" : "rs0",
+      "setVersion" : 1,
+      "isWritablePrimary" : true,
+      "secondary" : false,
+      "primary" : "localhost:27017",
+      "me" : "localhost:27017",
+      "electionId" : {"$oid" : "7fffffff0000000000000016"},
+      "lastWrite" : {
+         "opTime" : {"ts" : {"$timestamp" : {"t" : 1737682844, "i" : 1}}, "t" : 22},
+         "lastWriteDate" : {"$date" : "2025-01-24T01:40:44Z"},
+         "majorityOpTime" : {"ts" : {"$timestamp" : {"t" : 1737682844, "i" : 1}}, "t" : 22},
+         "majorityWriteDate" : {"$date" : "2025-01-24T01:40:44Z"}
+      },
+      "maxBsonObjectSize" : 16777216,
+      "maxMessageSizeBytes" : 48000000,
+      "maxWriteBatchSize" : 100000,
+      "localTime" : {"$date" : "2025-01-24T01:40:51.968Z"},
+      "logicalSessionTimeoutMinutes" : 30,
+      "connectionId" : 13,
+      "minWireVersion" : 0,
+      "maxWireVersion" : 25,
+      "readOnly" : false,
+      "ok" : 1.0,
+      "$clusterTime" : {
+         "clusterTime" : {"$timestamp" : {"t" : 1737682844, "i" : 1}},
+         "signature" :
+            {"hash" : {"$binary" : {"base64" : "AAAAAAAAAAAAAAAAAAAAAAAAAAA=", "subType" : "00"}}, "keyId" : 0}
+      },
+      "operationTime" : {"$timestamp" : {"t" : 1737682844, "i" : 1}}
+   });
+
+   const char *hello_mongos = BSON_STR ({
+      "isWritablePrimary" : true,
+      "msg" : "isdbgrid",
+      "topologyVersion" : {"processId" : {"$oid" : "6791af1181771f367602ec40"}, "counter" : 0},
+      "maxBsonObjectSize" : 16777216,
+      "maxMessageSizeBytes" : 48000000,
+      "maxWriteBatchSize" : 100000,
+      "localTime" : {"$date" : "2025-01-24T01:24:57.217Z"},
+      "logicalSessionTimeoutMinutes" : 30,
+      "connectionId" : 3310,
+      "maxWireVersion" : 25,
+      "minWireVersion" : 0,
+      "ok" : 1.0,
+      "$clusterTime" : {
+         "clusterTime" : {"$timestamp" : {"t" : 1737681896, "i" : 1}},
+         "signature" :
+            {"hash" : {"$binary" : {"base64" : "AAAAAAAAAAAAAAAAAAAAAAAAAAA=", "subType" : "00"}}, "keyId" : 0}
+      },
+      "operationTime" : {"$timestamp" : {"t" : 1737681896, "i" : 1}}
+   });
+
+   test_copy (hello_mongod);
+   test_copy (hello_mongos);
+}
+
 void
 test_server_description_install (TestSuite *suite)
 {
@@ -470,4 +583,5 @@ test_server_description_install (TestSuite *suite)
    TestSuite_Add (suite, "/server_description/legacy_hello_ok", test_server_description_legacy_hello_ok);
    TestSuite_Add (suite, "/server_description/connection_id", test_server_description_connection_id);
    TestSuite_Add (suite, "/server_description/hello_type_error", test_server_description_hello_type_error);
+   TestSuite_Add (suite, "/server_description/copy", test_server_description_copy);
 }