CDRIVER-5756 Coverity fixes #1867
Conversation
CID: 100057
CID: 100087
CID: 115840, 115848, 115851, 115853
CID: 134019
CID: 138986
CID: 138989
CID: 156373
CID: 157950
vector-of-bool
left a comment
vector-of-bool
left a comment
There was a problem hiding this comment.
A few comments and questions.
| bson_mutex_lock (&topology->tpld_modification_mtx); | ||
| for (size_t i = 0u; i < n_rtt_monitors; i++) { | ||
| server_monitor = mongoc_set_get_item (topology->rtt_monitors, i); | ||
| mongoc_server_monitor_request_shutdown (server_monitor); | ||
| } | ||
| bson_mutex_unlock (&topology->tpld_modification_mtx); |
There was a problem hiding this comment.
I'm not sure if this mutex is relevant here. Do you have a link to the Coverity warning related to these lines?
There was a problem hiding this comment.
I sent you the link in a DM.
There was a problem hiding this comment.
I strongly suspect there are existing thread-safety issues in _mongoc_topology_background_monitoring_stop.
I suspect between storing the lengths and accessing the sets, the set of server monitors could possibly change (via _mongoc_topology_background_monitoring_reconcile.
I recommend reverting this change. A more comprehensive fix can be done in the future to address the coverity warning.
Co-authored-by: Kevin Albertson <[email protected]>
|
Here is the patch build for the latest changes. Also, I have checked the Coverity dashboard and there are no new detections that need to be addressed. |
kevinAlbs
left a comment
kevinAlbs
left a comment
There was a problem hiding this comment.
Apologies for delayed review.
| bson_mutex_lock (&topology->tpld_modification_mtx); | ||
| for (size_t i = 0u; i < n_rtt_monitors; i++) { | ||
| server_monitor = mongoc_set_get_item (topology->rtt_monitors, i); | ||
| mongoc_server_monitor_request_shutdown (server_monitor); | ||
| } | ||
| bson_mutex_unlock (&topology->tpld_modification_mtx); |
There was a problem hiding this comment.
I strongly suspect there are existing thread-safety issues in _mongoc_topology_background_monitoring_stop.
I suspect between storing the lengths and accessing the sets, the set of server monitors could possibly change (via _mongoc_topology_background_monitoring_reconcile.
I recommend reverting this change. A more comprehensive fix can be done in the future to address the coverity warning.
Co-authored-by: Kevin Albertson <[email protected]>
Co-authored-by: Kevin Albertson <[email protected]>
|
Here is the patch build for the latest changes. |
kevinAlbs
left a comment
kevinAlbs
left a comment
There was a problem hiding this comment.
LGTM. Suggest running git merge master to fix conflicts on this branch prior to merging (it looks like some of the argument asserts were already added).
rcsanchez97
requested review from
eramongodb
and removed request for
eramongodb and
vector-of-bool
|
I discussed with @vector-of-bool in Slack and he said he doesn't feel like he needs to review this again, so I have removed the request for his review. |
3 participants
This PR fixes some small Coverity issues (w/ full Evergreen patch build).
@kevinAlbs I wasn't sure who would be available to review, so please feel free to adjust the reviewers as needed.