Skip to content

CDRIVER-5756 Coverity fixes #1867

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/common/src/common-b64.c
Original file line number Diff line number Diff line change
Expand Up @@ -263,14 +263,20 @@ static const uint8_t mongoc_b64rmap_invalid = 0xff;
#if defined(BSON_OS_UNIX)
#include <pthread.h>
#define mongoc_common_once_t pthread_once_t
#define mongoc_common_once pthread_once
#define mongoc_common_once(o, c) \
do { \
Assert (pthread_once ((o), (c)) == 0); \
} while (0)
#define MONGOC_COMMON_ONCE_FUN(n) void n (void)
#define MONGOC_COMMON_ONCE_RETURN return
#define MONGOC_COMMON_ONCE_INIT PTHREAD_ONCE_INIT
#else
#define mongoc_common_once_t INIT_ONCE
#define MONGOC_COMMON_ONCE_INIT INIT_ONCE_STATIC_INIT
#define mongoc_common_once(o, c) InitOnceExecuteOnce (o, c, NULL, NULL)
#define mongoc_common_once(o, c) \
do { \
Assert (InitOnceExecuteOnce (o, c, NULL, NULL) != 0); \
} while (0)
#define MONGOC_COMMON_ONCE_FUN(n) BOOL CALLBACK n (PINIT_ONCE _ignored_a, PVOID _ignored_b, PVOID *_ignored_c)
#define MONGOC_COMMON_ONCE_RETURN return true
#endif
Expand Down
3 changes: 2 additions & 1 deletion src/libbson/src/bson/bson-json.c
Original file line number Diff line number Diff line change
Expand Up @@ -620,9 +620,10 @@ _bson_json_read_integer (bson_json_reader_t *reader, uint64_t val, int64_t sign)

if (rs == BSON_JSON_REGULAR) {
BASIC_CB_BAIL_IF_NOT_NORMAL ("integer");
BSON_ASSERT (mlib_in_range (int, len));

if (val <= INT32_MAX || (sign == -1 && val <= (uint64_t) INT32_MAX + 1)) {
bson_append_int32 (STACK_BSON_CHILD, key, (int) len, (int) (val * sign));
bson_append_int32 (STACK_BSON_CHILD, key, (int) len, (int32_t) ((int64_t) val * sign));
} else if (sign == -1) {
#if defined(_WIN32) && !defined(__MINGW32__)
// Unary negation of unsigned integer is deliberate.
Expand Down
7 changes: 7 additions & 0 deletions src/libbson/src/jsonsl/jsonsl.c
Original file line number Diff line number Diff line change
Expand Up @@ -1149,8 +1149,15 @@ void jsonsl_jpr_match_state_init(jsonsl_t jsn,
return;
}
jsn->jprs = (jsonsl_jpr_t *)malloc(sizeof(jsonsl_jpr_t) * njprs);
if (!jsn->jprs) {
return;
}
Comment on lines 1151 to +1154
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
jsn->jprs = (jsonsl_jpr_t *)malloc(sizeof(jsonsl_jpr_t) * njprs);
if (!jsn->jprs) {
return;
}
jsn->jprs = (jsonsl_jpr_t *) bson_malloc (sizeof (jsonsl_jpr_t) * njprs);

Suggest using bson_malloc. bson_malloc already checks (and aborts) if allocation fails. I expect that will fix the Coverity warning. Using bson_malloc ensures allocators set with bson_mem_set_vtable are used.

Also replace free(jsn->jprs); with bson_free(jsn->jprs);

jsn->jpr_count = njprs;
jsn->jpr_root = (size_t*)calloc(1, sizeof(size_t) * njprs * jsn->levels_max);
if (!jsn->jpr_root) {
free(jsn->jprs);
return;
}
Comment on lines 1156 to +1160
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
jsn->jpr_root = (size_t*)calloc(1, sizeof(size_t) * njprs * jsn->levels_max);
if (!jsn->jpr_root) {
free(jsn->jprs);
return;
}
jsn->jpr_root = (size_t *) bson_malloc0 (sizeof (size_t) * njprs * jsn->levels_max);

Similarly, use bson_malloc0 in place of calloc. Suggest updating other malloc / free calls in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. That makes sense. Should I do that as a drive-by in this PR, or a separate PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK to include in this PR. Though the other malloc calls check their returns, I think it is a related enough change to include in this PR.

memcpy(jsn->jprs, jprs, sizeof(jsonsl_jpr_t) * njprs);
/* Set the initial jump table values */

Expand Down
4 changes: 3 additions & 1 deletion src/libmongoc/src/mongoc/mongoc-client-session.c
Original file line number Diff line number Diff line change
Expand Up @@ -886,7 +886,9 @@ _max_time_ms_failure (bson_t *reply)
return true;
}

bson_iter_init (&iter, reply);
if (!bson_iter_init (&iter, reply)) {
return false;
}
if (bson_iter_find_descendant (&iter, "writeConcernError.codeName", &descendant) &&
BSON_ITER_HOLDS_UTF8 (&descendant) && 0 == strcmp (bson_iter_utf8 (&descendant, NULL), MAX_TIME_MS_EXPIRED)) {
return true;
Expand Down
8 changes: 6 additions & 2 deletions src/libmongoc/src/mongoc/mongoc-collection.c
Original file line number Diff line number Diff line change
Expand Up @@ -1255,8 +1255,12 @@ _mongoc_collection_index_keys_equal (const bson_t *expected, const bson_t *actua
bson_iter_t iter_expected;
bson_iter_t iter_actual;

bson_iter_init (&iter_expected, expected);
bson_iter_init (&iter_actual, actual);
if (!bson_iter_init (&iter_expected, expected)) {
return false;
}
if (!bson_iter_init (&iter_actual, actual)) {
return false;
}

while (bson_iter_next (&iter_expected)) {
/* If the key document has fewer items than expected, indexes are unequal
Expand Down
4 changes: 3 additions & 1 deletion src/libmongoc/src/mongoc/mongoc-counters.c
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,9 @@ mongoc_counters_calc_size (void)
(n_cpu * n_groups * sizeof (mongoc_counter_slots_t)));

#ifdef BSON_OS_UNIX
return BSON_MAX (sysconf (_SC_PAGESIZE), size);
long pg_sz = sysconf (_SC_PAGESIZE);
BSON_ASSERT (pg_sz > 0);
return BSON_MAX ((size_t) pg_sz, size);
#else
return size;
#endif
Expand Down
2 changes: 1 addition & 1 deletion src/libmongoc/src/mongoc/mongoc-server-description.c
Original file line number Diff line number Diff line change
Expand Up @@ -790,7 +790,7 @@ mongoc_server_description_new_copy (const mongoc_server_description_t *descripti
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); \
BSON_ASSERT (bson_init_static (&copy->FIELD, data, len)); \
} else { \
bson_init (&copy->FIELD); \
} \
Expand Down
1 change: 1 addition & 0 deletions src/libmongoc/src/mongoc/mongoc-socket.c
Original file line number Diff line number Diff line change
Expand Up @@ -1207,6 +1207,7 @@ _mongoc_socket_try_sendv_slow (mongoc_socket_t *sock, /* IN */
RETURN (ret ? ret : -1);
}

BSON_ASSERT (mlib_cmp (wrote, <=, SSIZE_MAX - ret));
ret += wrote;

if (mlib_cmp (wrote, !=, iov[i].iov_len)) {
Expand Down
2 changes: 2 additions & 0 deletions src/libmongoc/src/mongoc/mongoc-stream-tls-openssl.c
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,8 @@ create_stream_with_ctx (
mongoc_stream_t *
mongoc_stream_tls_openssl_new (mongoc_stream_t *base_stream, const char *host, mongoc_ssl_opt_t *opt, int client)
{
BSON_ASSERT (opt);

SSL_CTX *ssl_ctx = _mongoc_openssl_ctx_new (opt);

if (!ssl_ctx) {
Expand Down
5 changes: 3 additions & 2 deletions src/libmongoc/src/mongoc/mongoc-stream-tls.c
Original file line number Diff line number Diff line change
Expand Up @@ -194,13 +194,13 @@ mongoc_stream_tls_new_with_hostname (mongoc_stream_t *base_stream, const char *h

/* !client is only used for testing,
* when the streams are pretending to be the server */
if (!client || opt->weak_cert_validation) {
if (opt && (!client || opt->weak_cert_validation)) {
opt->allow_invalid_hostname = true;
}

#ifndef _WIN32
/* Silly check for Unix Domain Sockets */
if (!host || (host[0] == '/' && !access (host, F_OK))) {
if (opt && (!host || (host[0] == '/' && !access (host, F_OK)))) {
opt->allow_invalid_hostname = true;
}
#endif
Expand Down Expand Up @@ -252,6 +252,7 @@ mongoc_stream_tls_new_with_hostname_and_openssl_context (
mongoc_stream_t *base_stream, const char *host, mongoc_ssl_opt_t *opt, int client, SSL_CTX *ssl_ctx)
{
BSON_ASSERT (base_stream);
BSON_ASSERT (opt);

/* !client is only used for testing,
* when the streams are pretending to be the server */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -311,10 +311,12 @@ _mongoc_topology_background_monitoring_stop (mongoc_topology_t *topology)
}

/* Signal all RTT monitors to shut down. */
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);
Comment on lines +314 to +319
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this mutex is relevant here. Do you have a link to the Coverity warning related to these lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sent you the link in a DM.


for (size_t i = 0u; i < n_srv_monitors; i++) {
/* Wait for the thread to shutdown. */
Expand Down
10 changes: 8 additions & 2 deletions src/libmongoc/src/mongoc/mongoc-uri.c
Original file line number Diff line number Diff line change
Expand Up @@ -953,7 +953,10 @@ mongoc_uri_options_validate_names (const bson_t *a, const bson_t *b, bson_error_
/* Scan `a` looking for deprecated names
* where the canonical name was also used in `a`,
* or was used in `b`. */
bson_iter_init (&key_iter, a);
if (!bson_iter_init (&key_iter, a)) {
return false;
}

while (bson_iter_next (&key_iter)) {
key = bson_iter_key (&key_iter);
value = bson_iter_utf8_unsafe (&key_iter, &value_len);
Expand Down Expand Up @@ -1017,7 +1020,10 @@ mongoc_uri_apply_options (mongoc_uri_t *uri, const bson_t *options, bool from_dn
size_t value_len;
bool bval;

bson_iter_init (&iter, options);
if (!bson_iter_init (&iter, options)) {
return false;
}

while (bson_iter_next (&iter)) {
key = bson_iter_key (&iter);
canon = mongoc_uri_canonicalize_option (key);
Expand Down
2 changes: 2 additions & 0 deletions src/tools/mongoc-stat.c
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <sys/stat.h>
#include <unistd.h>

#include <mlib/cmp.h>

#pragma pack(1)
typedef struct {
Expand Down Expand Up @@ -108,6 +109,7 @@ mongoc_counters_new_from_pid (unsigned pid)
return NULL;
}

BSON_ASSERT (mlib_in_range (size_t, len));
size = len;

if (MAP_FAILED == (mem = mmap (NULL, size, PROT_READ, MAP_SHARED, fd, 0))) {
Expand Down