From f807b9bafc73cc14aa23852edbb42bcf8c792f33 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Jan 12 2018 13:40:10 +0000 Subject: Ticket 49529 - Fix Coverity warnings: invalid deferences Description: So many of the warnings were false positives, but I "fixed" 90% of them anyway for these two reasons: One, it's possible that a future change could actually result in a NULL pointer being referenced. Two, it would be nice to stop these coverity warnings so we can focus on real warnings. Auto waivers also don't always work as the surrounding code changes. https://pagure.io/389-ds-base/issue/49529 Reviewed by: firstyear (Thanks!) (cherry picked from commit 7e27face5ef021d883a44d70bb3e9732b115016f) --- diff --git a/ldap/servers/slapd/abandon.c b/ldap/servers/slapd/abandon.c index 5c30c97..e2237e5 100644 --- a/ldap/servers/slapd/abandon.c +++ b/ldap/servers/slapd/abandon.c @@ -42,10 +42,16 @@ do_abandon(Slapi_PBlock *pb) slapi_pblock_get(pb, SLAPI_OPERATION, &pb_op); slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); - BerElement *ber = pb_op->o_ber; - slapi_log_err(SLAPI_LOG_TRACE, "do_abandon", "->\n"); + if (pb_op == NULL || pb_conn == NULL) { + slapi_log_err(SLAPI_LOG_ERR, "do_abandon", "NULL param: pb_conn (0x%p) pb_op (0x%p)\n", + pb_conn, pb_op); + return; + } + + BerElement *ber = pb_op->o_ber; + /* * Parse the abandon request. It looks like this: * diff --git a/ldap/servers/slapd/add.c b/ldap/servers/slapd/add.c index 0a4a5d7..8f2fdea 100644 --- a/ldap/servers/slapd/add.c +++ b/ldap/servers/slapd/add.c @@ -66,6 +66,14 @@ do_add(Slapi_PBlock *pb) slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); slapi_pblock_get(pb, SLAPI_OPERATION, &operation); + + + if (operation == NULL || pb_conn == NULL) { + slapi_log_err(SLAPI_LOG_ERR, "do_add", "NULL param: pb_conn (0x%p) pb_op (0x%p)\n", + pb_conn, operation); + send_ldap_result(pb, LDAP_OPERATIONS_ERROR, NULL, "param error", 0, NULL); + return; + } ber = operation->o_ber; /* count the add request */ @@ -450,8 +458,8 @@ op_shared_add(Slapi_PBlock *pb) if (!internal_op) { slapi_log_access(LDAP_DEBUG_STATS, "conn=%" PRIu64 " op=%d ADD dn=\"%s\"%s\n", - pb_conn->c_connid, - operation->o_opid, + pb_conn ? pb_conn->c_connid : -1, + operation ? operation->o_opid: -1, slapi_entry_get_dn_const(e), proxystr ? proxystr : ""); } else { @@ -865,7 +873,11 @@ handle_fast_add(Slapi_PBlock *pb, Slapi_Entry *entry) int ret; slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); - + if (pb_conn == NULL){ + slapi_log_err(SLAPI_LOG_ERR, "handle_fast_add", "NULL param: pb_conn (0x%p)\n", pb_conn); + send_ldap_result(pb, LDAP_OPERATIONS_ERROR, NULL, "param error", 0, NULL); + return; + } be = pb_conn->c_bi_backend; if ((be == NULL) || (be->be_wire_import == NULL)) { diff --git a/ldap/servers/slapd/bind.c b/ldap/servers/slapd/bind.c index 4a8e4de..a34a21a 100644 --- a/ldap/servers/slapd/bind.c +++ b/ldap/servers/slapd/bind.c @@ -54,11 +54,7 @@ do_bind(Slapi_PBlock *pb) { Operation *pb_op = NULL; Connection *pb_conn = NULL; - - slapi_pblock_get(pb, SLAPI_OPERATION, &pb_op); - slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); - - BerElement *ber = pb_op->o_ber; + BerElement *ber; int err, isroot; ber_tag_t method = LBER_DEFAULT; ber_int_t version = -1; @@ -83,6 +79,16 @@ do_bind(Slapi_PBlock *pb) slapi_log_err(SLAPI_LOG_TRACE, "do_bind", "=>\n"); + slapi_pblock_get(pb, SLAPI_OPERATION, &pb_op); + slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); + if (pb_op == NULL || pb_conn == NULL) { + slapi_log_err(SLAPI_LOG_ERR, "do_bind", "NULL param: pb_conn (0x%p) pb_op (0x%p)\n", + pb_conn, pb_op); + send_ldap_result(pb, LDAP_OPERATIONS_ERROR, NULL, NULL, 0, NULL); + goto free_and_return; + } + ber = pb_op->o_ber; + /* * Parse the bind request. It looks like this: * @@ -856,6 +862,10 @@ log_bind_access( slapi_pblock_get(pb, SLAPI_OPERATION, &pb_op); slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); + if (pb_op == NULL || pb_conn == NULL) { + return; + } + if (method == LDAP_AUTH_SASL && saslmech && msg) { slapi_log_access(LDAP_DEBUG_STATS, "conn=%" PRIu64 " op=%d BIND dn=\"%s\" " diff --git a/ldap/servers/slapd/compare.c b/ldap/servers/slapd/compare.c index 9bc6b69..2626d91 100644 --- a/ldap/servers/slapd/compare.c +++ b/ldap/servers/slapd/compare.c @@ -35,10 +35,7 @@ do_compare(Slapi_PBlock *pb) { Operation *pb_op = NULL; Connection *pb_conn = NULL; - slapi_pblock_get(pb, SLAPI_OPERATION, &pb_op); - slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); - - BerElement *ber = pb_op->o_ber; + BerElement *ber; char *rawdn = NULL; const char *dn = NULL; struct ava ava = {0}; @@ -50,6 +47,18 @@ do_compare(Slapi_PBlock *pb) slapi_log_err(SLAPI_LOG_TRACE, "do_compare", "=>\n"); + slapi_pblock_get(pb, SLAPI_OPERATION, &pb_op); + slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); + + if (pb_op == NULL || pb_conn == NULL) { + slapi_log_err(SLAPI_LOG_ERR, "do_compare", "NULL param: pb_conn (0x%p) pb_op (0x%p)\n", + pb_conn, pb_op); + send_ldap_result(pb, LDAP_OPERATIONS_ERROR, NULL, NULL, 0, NULL); + goto free_and_return; + } + + ber = pb_op->o_ber; + /* count the compare request */ slapi_counter_increment(g_get_global_snmp_vars()->ops_tbl.dsCompareOps); diff --git a/ldap/servers/slapd/connection.c b/ldap/servers/slapd/connection.c index 8ef1156..fa24ec0 100644 --- a/ldap/servers/slapd/connection.c +++ b/ldap/servers/slapd/connection.c @@ -1518,7 +1518,7 @@ connection_threadmain() } if (!thread_turbo_flag && !more_data) { - Connection *pb_conn = NULL; + Connection *pb_conn = NULL; /* If more data is left from the previous connection_read_operation, we should finish the op now. Client might be thinking it's @@ -1530,6 +1530,13 @@ connection_threadmain() * Connection wait for new work provides the conn and op for us. */ slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); + if (pb_conn == NULL) { + slapi_log_err(SLAPI_LOG_ERR, "connection_threadmain", + "pb_conn is NULL\n"); + slapi_pblock_destroy(pb); + g_decr_active_threadcnt(); + return; + } switch (ret) { case CONN_NOWORK: @@ -1702,11 +1709,11 @@ connection_threadmain() * so need locking from here on */ signal_listner(); /* with nunc-stans, I see an enormous amount of time spent in the poll() in - * connection_read_operation() when the below code is enabled - not sure why - * nunc-stans makes such a huge difference - for now, just disable this code - * when using nunc-stans - it is supposed to be an optimization but turns out - * to not be the opposite with nunc-stans - */ + * connection_read_operation() when the below code is enabled - not sure why + * nunc-stans makes such a huge difference - for now, just disable this code + * when using nunc-stans - it is supposed to be an optimization but turns out + * to not be the opposite with nunc-stans + */ } else if (!enable_nunc_stans) { /* more data in conn - just put back on work_q - bypass poll */ bypasspollcnt++; PR_EnterMonitor(conn->c_mutex); diff --git a/ldap/servers/slapd/delete.c b/ldap/servers/slapd/delete.c index ba238b1..49cdab1 100644 --- a/ldap/servers/slapd/delete.c +++ b/ldap/servers/slapd/delete.c @@ -262,8 +262,8 @@ op_shared_delete(Slapi_PBlock *pb) slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); slapi_pblock_get(pb, SLAPI_OPERATION, &pb_op); slapi_log_access(LDAP_DEBUG_STATS, "conn=%" PRIu64 " op=%d DEL dn=\"%s\"%s\n", - pb_conn->c_connid, - pb_op->o_opid, + pb_conn ? pb_conn->c_connid : -1, + pb_op ? pb_op->o_opid : -1, slapi_sdn_get_dn(sdn), proxystr ? proxystr : ""); } else { diff --git a/ldap/servers/slapd/dn.c b/ldap/servers/slapd/dn.c index afca372..abc1555 100644 --- a/ldap/servers/slapd/dn.c +++ b/ldap/servers/slapd/dn.c @@ -2477,6 +2477,13 @@ slapi_sdn_copy(const Slapi_DN *from, Slapi_DN *to) { SDN_DUMP(from, "slapi_sdn_copy from"); SDN_DUMP(to, "slapi_sdn_copy to"); + + if (to == NULL || from == NULL){ + slapi_log_err(SLAPI_LOG_ERR, "slapi_sdn_copy", + "NULL param: from (0x%p) to (0x%p)\n", from, to); + return; + } + slapi_sdn_done(to); if (from->udn) { to->flag = slapi_setbit_uchar(to->flag, FLAG_UDN); diff --git a/ldap/servers/slapd/entry.c b/ldap/servers/slapd/entry.c index fbbc8fa..32828b4 100644 --- a/ldap/servers/slapd/entry.c +++ b/ldap/servers/slapd/entry.c @@ -1998,6 +1998,10 @@ slapi_entry_dup(const Slapi_Entry *e) struct attrs_in_extension *aiep; PR_ASSERT(NULL != e); + if (e == NULL){ + slapi_log_err(SLAPI_LOG_ERR, "slapi_entry_dup", "entry is NULL\n"); + return NULL; + } ec = slapi_entry_alloc(); @@ -3660,7 +3664,11 @@ delete_values_sv_internal( Slapi_Attr *a; int retVal = LDAP_SUCCESS; -/* + if (e == NULL){ + slapi_log_err(SLAPI_LOG_ERR, "delete_values_sv_internal", "entry is NULL\n"); + return LDAP_OPERATIONS_ERROR; + } + /* * If type is in the protected_attrs_all list, we could ignore the failure, * as the attribute could only exist in the entry in the memory when the * add/mod operation is done, while the retried entry from the db does not diff --git a/ldap/servers/slapd/extendop.c b/ldap/servers/slapd/extendop.c index 1594a8c..815949b 100644 --- a/ldap/servers/slapd/extendop.c +++ b/ldap/servers/slapd/extendop.c @@ -219,6 +219,13 @@ do_extended(Slapi_PBlock *pb) slapi_pblock_get(pb, SLAPI_OPERATION, &pb_op); slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); + if (pb_conn == NULL || pb_op == NULL) { + send_ldap_result(pb, LDAP_OPERATIONS_ERROR, NULL, "param error", 0, NULL); + slapi_log_err(SLAPI_LOG_ERR, "do_extended", + "NULL param error: conn (0x%p) op (0x%p)\n", pb_conn, pb_op); + goto free_and_return; + } + /* * Parse the extended request. It looks like this: * diff --git a/ldap/servers/slapd/filter.c b/ldap/servers/slapd/filter.c index fe3525f..ef975e6 100644 --- a/ldap/servers/slapd/filter.c +++ b/ldap/servers/slapd/filter.c @@ -292,7 +292,11 @@ get_filter_internal(Connection *conn, BerElement *ber, struct slapi_filter **fil case LDAP_FILTER_EXTENDED: slapi_log_err(SLAPI_LOG_FILTER, "get_filter_internal", "EXTENDED\n"); - if (conn->c_ldapversion < 3) { + if (conn == NULL) { + slapi_log_err(SLAPI_LOG_ERR, "get_filter_internal", + "NULL param: conn (0x%p)\n", conn); + err = LDAP_OPERATIONS_ERROR; + } else if (conn->c_ldapversion < 3) { slapi_log_err(SLAPI_LOG_ERR, "get_filter_internal", "Extensible filter received from v2 client\n"); err = LDAP_PROTOCOL_ERROR; diff --git a/ldap/servers/slapd/modify.c b/ldap/servers/slapd/modify.c index 0dcac64..10d2631 100644 --- a/ldap/servers/slapd/modify.c +++ b/ldap/servers/slapd/modify.c @@ -122,9 +122,16 @@ do_modify(Slapi_PBlock *pb) slapi_log_err(SLAPI_LOG_TRACE, "do_modify", "=>\n"); slapi_pblock_get(pb, SLAPI_OPERATION, &operation); - ber = operation->o_ber; - slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); + if (operation == NULL) { + send_ldap_result(pb, LDAP_OPERATIONS_ERROR, + NULL, "operation is NULL parameter", 0, NULL); + slapi_log_err(SLAPI_LOG_ERR, "do_modify", + "NULL param: pb_conn (0x%p) operation (0x%p)\n", pb_conn, operation); + return; + } + + ber = operation->o_ber; /* count the modify request */ slapi_counter_increment(g_get_global_snmp_vars()->ops_tbl.dsModifyEntryOps); @@ -1165,6 +1172,13 @@ op_shared_allow_pw_change(Slapi_PBlock *pb, LDAPMod *mod, char **old_pw, Slapi_M internal_op = operation_is_flag_set(operation, OP_FLAG_INTERNAL); slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); + if (pb_conn == NULL || operation == NULL) { + slapi_log_err(SLAPI_LOG_ERR, "op_shared_allow_pw_change", + "NULL param error: conn (0x%p) op (0x%p)\n", pb_conn, operation); + rc = -1; + goto done; + } + slapi_sdn_init_dn_byref(&sdn, dn); pwpolicy = new_passwdPolicy(pb, (char *)slapi_sdn_get_ndn(&sdn)); diff --git a/ldap/servers/slapd/passwd_extop.c b/ldap/servers/slapd/passwd_extop.c index 54a9a67..40145af 100644 --- a/ldap/servers/slapd/passwd_extop.c +++ b/ldap/servers/slapd/passwd_extop.c @@ -486,6 +486,10 @@ passwd_modify_extop(Slapi_PBlock *pb) /* Allow password modify only for SSL/TLS established connections and * connections using SASL privacy layers */ slapi_pblock_get(pb, SLAPI_CONNECTION, &conn); + if (conn == NULL) { + slapi_log_err(SLAPI_LOG_ERR, "passwd_modify_extop", "conn is NULL"); + goto free_and_return; + } if (slapi_pblock_get(pb, SLAPI_CONN_SASL_SSF, &sasl_ssf) != 0) { errMesg = "Could not get SASL SSF from connection\n"; rc = LDAP_OPERATIONS_ERROR; diff --git a/ldap/servers/slapd/psearch.c b/ldap/servers/slapd/psearch.c index e0dd2bf..1bf0629 100644 --- a/ldap/servers/slapd/psearch.c +++ b/ldap/servers/slapd/psearch.c @@ -271,6 +271,11 @@ ps_send_results(void *arg) slapi_pblock_get(ps->ps_pblock, SLAPI_CONNECTION, &pb_conn); slapi_pblock_get(ps->ps_pblock, SLAPI_OPERATION, &pb_op); + if (pb_conn == NULL) { + slapi_log_err(SLAPI_LOG_ERR, "ps_send_results", "pb_conn is NULL\n"); + return; + } + /* need to acquire a reference to this connection so that it will not be released or cleaned up out from under us */ PR_EnterMonitor(pb_conn->c_mutex); @@ -280,7 +285,7 @@ ps_send_results(void *arg) if (conn_acq_flag) { slapi_log_err(SLAPI_LOG_CONNS, "ps_send_results", "conn=%" PRIu64 " op=%d Could not acquire the connection - psearch aborted\n", - pb_conn->c_connid, pb_op->o_opid); + pb_conn->c_connid, pb_op ? pb_op->o_opid : -1); } PR_Lock(psearch_list->pl_cvarlock); @@ -290,7 +295,7 @@ ps_send_results(void *arg) if (pb_op == NULL || slapi_op_abandoned(ps->ps_pblock)) { slapi_log_err(SLAPI_LOG_CONNS, "ps_send_results", "conn=%" PRIu64 " op=%d The operation has been abandoned\n", - pb_conn->c_connid, pb_op->o_opid); + pb_conn->c_connid, pb_op ? pb_op->o_opid : -1); break; } if (NULL == ps->ps_eq_head) { @@ -532,7 +537,7 @@ ps_service_persistent_searches(Slapi_Entry *e, Slapi_Entry *eprev, ber_int_t chg slapi_log_err(SLAPI_LOG_CONNS, "ps_service_persistent_searches", "conn=%" PRIu64 " op=%d entry %s with chgtype %d " "matches the ps changetype %d\n", - pb_conn->c_connid, + pb_conn ? pb_conn->c_connid : -1, pb_op->o_opid, edn, chgtype, ps->ps_changetypes); @@ -609,7 +614,7 @@ ps_service_persistent_searches(Slapi_Entry *e, Slapi_Entry *eprev, ber_int_t chg /* Turn 'em loose */ ps_wakeup_all(); slapi_log_err(SLAPI_LOG_TRACE, "ps_service_persistent_searches", "Enqueued entry " - "\"%s\" on %d persistent search lists\n", + "\"%s\" on %d persistent search lists\n", slapi_entry_get_dn_const(e), matched); } else { slapi_log_err(SLAPI_LOG_TRACE, "ps_service_persistent_searches", diff --git a/ldap/servers/slapd/result.c b/ldap/servers/slapd/result.c index 2302ae9..ce394d9 100644 --- a/ldap/servers/slapd/result.c +++ b/ldap/servers/slapd/result.c @@ -396,7 +396,7 @@ send_ldap_result_ext( break; case LDAP_REFERRAL: - if (conn->c_ldapversion > LDAP_VERSION2) { + if (conn && conn->c_ldapversion > LDAP_VERSION2) { tag = LDAP_TAG_REFERRAL; break; } @@ -645,6 +645,11 @@ process_read_entry_controls(Slapi_PBlock *pb, char *oid) BerElement *req_ber = NULL; Operation *op = NULL; slapi_pblock_get(pb, SLAPI_OPERATION, &op); + if (op == NULL) { + slapi_log_err(SLAPI_LOG_ERR, "process_read_entry_controls", "op is NULL\n"); + rc = -1; + goto done; + } if (strcmp(oid, LDAP_CONTROL_PRE_READ_ENTRY) == 0) { /* first verify this is the correct operation for a pre-read entry control */ @@ -2145,6 +2150,13 @@ encode_read_entry(Slapi_PBlock *pb, Slapi_Entry *e, char **attrs, int alluseratt slapi_pblock_get(pb, SLAPI_OPERATION, &op); slapi_pblock_get(pb, SLAPI_CONNECTION, &conn); + if (conn == NULL || op == NULL) { + slapi_log_err(SLAPI_LOG_ERR, "encode_read_entry", + "NULL param error: conn (0x%p) op (0x%p)\n", conn, op); + rc = -1; + goto cleanup; + } + /* Start the ber encoding with the DN */ rc = ber_printf(ber, "t{s{", LDAP_RES_SEARCH_ENTRY, slapi_entry_get_dn_const(e)); if (rc == -1) { diff --git a/ldap/servers/slapd/search.c b/ldap/servers/slapd/search.c index 5e34132..731c651 100644 --- a/ldap/servers/slapd/search.c +++ b/ldap/servers/slapd/search.c @@ -125,7 +125,10 @@ do_search(Slapi_PBlock *pb) goto free_and_return; } - slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); + if (slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn) != 0 || pb_conn == NULL) { + slapi_log_err(SLAPI_LOG_ERR, "do_search", "pb_conn is NULL\n"); + goto free_and_return; + } /* * If nsslapd-minssf-exclude-rootdse is on, the minssf check has been diff --git a/ldap/servers/slapd/task.c b/ldap/servers/slapd/task.c index 53a0af5..002083c 100644 --- a/ldap/servers/slapd/task.c +++ b/ldap/servers/slapd/task.c @@ -199,6 +199,11 @@ slapi_task_log_status(Slapi_Task *task, char *format, ...) { va_list ap; + if (task == NULL) { + slapi_log_err(SLAPI_LOG_ERR, "slapi_task_log_status", + "Slapi_Task is NULL, can not log status\n"); + return; + } if (!task->task_status) task->task_status = (char *)slapi_ch_malloc(10 * LOG_BUFFER); if (!task->task_status)