From 86cb4a8568160e5f5f1051506b8f24bb1312ff60 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Mon, 24 Nov 2014 12:22:34 -0500 Subject: [PATCH 54/55] Ticket 47965 - Fix coverity issues (2014/11/24) 12864 - nesting level adjustment - sync_presist.c 12867 - Uninitialized pointer read - repl5_replica.c 12870 - Unused value - repl5_ruv.c 12880, 12881, 12893 - Unused value - dblayer.c 12887 - nesting level adjustment - acl.c 12894 - nesting level adjustment - acl_ext.c 12898 - Logically dead code - acllas.c 12891 - Unused value - windows_inc_protocol.c 12902 - Unused value - repl5_inc_protocol.c https://fedorahosted.org/389/ticket/47965 Reviewed by: rmeggins(Thanks!) (cherry picked from commit da90d57c30bf93265f06499b8ea2102378a0ed12) (cherry picked from commit 61d1211f1721355d571d570a67f36ef6baeb4866) --- ldap/servers/plugins/acl/acl.c | 3 +- ldap/servers/plugins/acl/acl_ext.c | 52 +++++++------ ldap/servers/plugins/acl/acllas.c | 4 - .../plugins/replication/repl5_inc_protocol.c | 1 - ldap/servers/plugins/replication/repl5_replica.c | 2 +- ldap/servers/plugins/replication/repl5_ruv.c | 1 - .../plugins/replication/windows_inc_protocol.c | 89 +++++++++++----------- ldap/servers/plugins/sync/sync_persist.c | 8 +- ldap/servers/slapd/back-ldbm/dblayer.c | 21 +++-- ldap/servers/slapd/pw.c | 47 +++++++----- 10 files changed, 119 insertions(+), 109 deletions(-) diff --git a/ldap/servers/plugins/acl/acl.c b/ldap/servers/plugins/acl/acl.c index 403c5b3..f04e258 100644 --- a/ldap/servers/plugins/acl/acl.c +++ b/ldap/servers/plugins/acl/acl.c @@ -2106,9 +2106,10 @@ acl__resource_match_aci( Acl_PBlock *aclpb, aci_t *aci, int skip_attrEval, int * ** acl in the entry cache list. */ if (!((res_right & (SLAPI_ACL_SEARCH | SLAPI_ACL_READ)) && - (aci_right & (SLAPI_ACL_SEARCH | SLAPI_ACL_READ)))) + (aci_right & (SLAPI_ACL_SEARCH | SLAPI_ACL_READ)))){ matches = ACL_FALSE; goto acl__resource_match_aci_EXIT; + } } diff --git a/ldap/servers/plugins/acl/acl_ext.c b/ldap/servers/plugins/acl/acl_ext.c index 126a234..0863de0 100644 --- a/ldap/servers/plugins/acl/acl_ext.c +++ b/ldap/servers/plugins/acl/acl_ext.c @@ -334,12 +334,14 @@ acl_operation_ext_destructor ( void *ext, void *object, void *parent ) int attr_only = 0; PRLock *shared_lock = aclcb->aclcb_lock; - if (aclcb->aclcb_lock ) PR_Lock ( shared_lock ); + if (aclcb->aclcb_lock ) + PR_Lock ( shared_lock ); else { goto clean_aclpb; } if ( !aclcb->aclcb_lock ) { - slapi_log_error (SLAPI_LOG_FATAL, plugin_name, "aclcb lock released! aclcb cache can't be refreshed\n"); + slapi_log_error (SLAPI_LOG_FATAL, plugin_name, + "aclcb lock released! aclcb cache can't be refreshed\n"); PR_Unlock ( shared_lock ); goto clean_aclpb; } @@ -347,29 +349,29 @@ acl_operation_ext_destructor ( void *ext, void *object, void *parent ) /* We need to refresh the aclcb cache */ if ( aclpb->aclpb_state & ACLPB_UPD_ACLCB_CACHE ) acl_clean_aclEval_context ( &aclcb->aclcb_eval_context, 0 /* clean*/ ); - if ( aclpb->aclpb_prev_entryEval_context.acle_numof_attrs ) { - c_evalContext = &aclpb->aclpb_prev_entryEval_context; - } else { - c_evalContext = &aclpb->aclpb_curr_entryEval_context; - } - - if (( aclpb->aclpb_state & ACLPB_INCR_ACLCB_CACHE ) && - ! ( aclpb->aclpb_state & ACLPB_UPD_ACLCB_CACHE )) - attr_only = 1; - - acl_copyEval_context ( NULL, c_evalContext, &aclcb->aclcb_eval_context, attr_only ); - - aclcb->aclcb_aclsignature = aclpb->aclpb_signature; - if ( aclcb->aclcb_sdn && - (0 != slapi_sdn_compare ( aclcb->aclcb_sdn, - aclpb->aclpb_authorization_sdn ) ) ) { - slapi_sdn_set_ndn_byval( aclcb->aclcb_sdn, - slapi_sdn_get_ndn ( aclpb->aclpb_authorization_sdn ) ); - } - aclcb->aclcb_state = 0; - aclcb->aclcb_state |= ACLCB_HAS_CACHED_EVALCONTEXT; - - PR_Unlock ( shared_lock ); + if ( aclpb->aclpb_prev_entryEval_context.acle_numof_attrs ) { + c_evalContext = &aclpb->aclpb_prev_entryEval_context; + } else { + c_evalContext = &aclpb->aclpb_curr_entryEval_context; + } + + if (( aclpb->aclpb_state & ACLPB_INCR_ACLCB_CACHE ) && + ! ( aclpb->aclpb_state & ACLPB_UPD_ACLCB_CACHE )) + attr_only = 1; + + acl_copyEval_context ( NULL, c_evalContext, &aclcb->aclcb_eval_context, attr_only ); + + aclcb->aclcb_aclsignature = aclpb->aclpb_signature; + if ( aclcb->aclcb_sdn && + (0 != slapi_sdn_compare( aclcb->aclcb_sdn, aclpb->aclpb_authorization_sdn ))) + { + slapi_sdn_set_ndn_byval( aclcb->aclcb_sdn, + slapi_sdn_get_ndn ( aclpb->aclpb_authorization_sdn ) ); + } + aclcb->aclcb_state = 0; + aclcb->aclcb_state |= ACLCB_HAS_CACHED_EVALCONTEXT; + + PR_Unlock ( shared_lock ); } clean_aclpb: diff --git a/ldap/servers/plugins/acl/acllas.c b/ldap/servers/plugins/acl/acllas.c index 2d0dae9..439c8f1 100644 --- a/ldap/servers/plugins/acl/acllas.c +++ b/ldap/servers/plugins/acl/acllas.c @@ -1726,10 +1726,6 @@ DS_LASAuthMethodEval(NSErr_t *errp, char *attr_name, CmpOp_t comparator, } else { rc = (matched == ACL_TRUE ? LAS_EVAL_FALSE : LAS_EVAL_TRUE); } - } else { - rc = LAS_EVAL_FAIL; - slapi_log_error( SLAPI_LOG_ACL, plugin_name, - "Returning UNDEFINED for authmethod evaluation.\n"); } return rc; diff --git a/ldap/servers/plugins/replication/repl5_inc_protocol.c b/ldap/servers/plugins/replication/repl5_inc_protocol.c index b867fc4..5cf170c 100644 --- a/ldap/servers/plugins/replication/repl5_inc_protocol.c +++ b/ldap/servers/plugins/replication/repl5_inc_protocol.c @@ -940,7 +940,6 @@ repl5_inc_run(Private_Repl_Protocol *prp) /* Destroy the backoff timer, since we won't need it anymore */ backoff_delete(&prp_priv->backoff); } - use_busy_backoff_timer = PR_FALSE; } else if (event_occurred(prp, EVENT_TRIGGERING_CRITERIA_MET)){ /* changes are available */ if ( prp_priv->backoff == NULL || backoff_expired (prp_priv->backoff, 60)){ diff --git a/ldap/servers/plugins/replication/repl5_replica.c b/ldap/servers/plugins/replication/repl5_replica.c index dab1c4e..77663f6 100644 --- a/ldap/servers/plugins/replication/repl5_replica.c +++ b/ldap/servers/plugins/replication/repl5_replica.c @@ -2140,7 +2140,7 @@ done: char *ridstr = NULL; char *token = NULL; char *repl_root; - char *iter; + char *iter = NULL; int i; for(i = 0; clean_vals[i]; i++){ diff --git a/ldap/servers/plugins/replication/repl5_ruv.c b/ldap/servers/plugins/replication/repl5_ruv.c index 500fd7a..132c641 100644 --- a/ldap/servers/plugins/replication/repl5_ruv.c +++ b/ldap/servers/plugins/replication/repl5_ruv.c @@ -705,7 +705,6 @@ set_max_csn_nolock_ext(RUV *ruv, const CSN *max_csn, const char *replica_purl, P csn_as_string(replica->csn, PR_FALSE, csn2)); return_value = RUV_COVERS_CSN; } - return_value = RUV_SUCCESS; } return return_value; } diff --git a/ldap/servers/plugins/replication/windows_inc_protocol.c b/ldap/servers/plugins/replication/windows_inc_protocol.c index d62deec..ec6ca55 100644 --- a/ldap/servers/plugins/replication/windows_inc_protocol.c +++ b/ldap/servers/plugins/replication/windows_inc_protocol.c @@ -675,63 +675,64 @@ windows_inc_run(Private_Repl_Protocol *prp) backoff_delete(&prp_priv->backoff); } else if (event_occurred(prp, EVENT_BACKOFF_EXPIRED)) - { + { rc = windows_acquire_replica(prp, &ruv, 1 /* check RUV for incremental */); use_busy_backoff_timer = PR_FALSE; if (rc == ACQUIRE_SUCCESS) - { - next_state = STATE_SENDING_UPDATES; - } + { + next_state = STATE_SENDING_UPDATES; + } else if (rc == ACQUIRE_REPLICA_BUSY) - { - next_state = STATE_BACKOFF; - use_busy_backoff_timer = PR_TRUE; - } + { + next_state = STATE_BACKOFF; + use_busy_backoff_timer = PR_TRUE; + } else if (rc == ACQUIRE_CONSUMER_WAS_UPTODATE) - { + { next_state = STATE_WAIT_CHANGES; - } + } else if (rc == ACQUIRE_TRANSIENT_ERROR) - { - next_state = STATE_BACKOFF; - } + { + next_state = STATE_BACKOFF; + } else if (rc == ACQUIRE_FATAL_ERROR) - { - next_state = STATE_STOP_FATAL_ERROR; - } + { + next_state = STATE_STOP_FATAL_ERROR; + } + if (rc != ACQUIRE_SUCCESS) - { - int optype, ldaprc; - windows_conn_get_error(prp->conn, &optype, &ldaprc); - agmt_set_last_update_status(prp->agmt, ldaprc, + { + int optype, ldaprc; + windows_conn_get_error(prp->conn, &optype, &ldaprc); + agmt_set_last_update_status(prp->agmt, ldaprc, prp->last_acquire_response_code, NULL); - } - /* - * We either need to step the backoff timer, or - * destroy it if we don't need it anymore. - */ + } + + /* + * We either need to step the backoff timer, or + * destroy it if we don't need it anymore. + */ if (STATE_BACKOFF == next_state) - { - time_t next_fire_time; - time_t now; - /* Step the backoff timer */ - time(&now); - next_fire_time = backoff_step(prp_priv->backoff); - /* And go back to sleep */ - slapi_log_error(SLAPI_LOG_REPL, windows_repl_plugin_name, - "%s: Replication session backing off for %ld seconds\n", - agmt_get_long_name(prp->agmt), - next_fire_time - now); + { + time_t next_fire_time; + time_t now; + /* Step the backoff timer */ + time(&now); + next_fire_time = backoff_step(prp_priv->backoff); + /* And go back to sleep */ + slapi_log_error(SLAPI_LOG_REPL, windows_repl_plugin_name, + "%s: Replication session backing off for %ld seconds\n", + agmt_get_long_name(prp->agmt), + next_fire_time - now); - protocol_sleep(prp, PR_INTERVAL_NO_TIMEOUT); - } + protocol_sleep(prp, PR_INTERVAL_NO_TIMEOUT); + } else - { - /* Destroy the backoff timer, since we won't need it anymore */ - backoff_delete(&prp_priv->backoff); - } - use_busy_backoff_timer = PR_FALSE; - } + { + /* Destroy the backoff timer, since we won't need it anymore */ + backoff_delete(&prp_priv->backoff); + } + } else if (event_occurred(prp, EVENT_TRIGGERING_CRITERIA_MET)) { /* changes are available */ diff --git a/ldap/servers/plugins/sync/sync_persist.c b/ldap/servers/plugins/sync/sync_persist.c index 8d46102..f41a10c 100644 --- a/ldap/servers/plugins/sync/sync_persist.c +++ b/ldap/servers/plugins/sync/sync_persist.c @@ -661,10 +661,10 @@ sync_send_results( void *arg ) ectrls = (LDAPControl **)slapi_ch_calloc(2, sizeof (LDAPControl *)); if (req->req_cookie) sync_cookie_update(req->req_cookie, ec); - sync_create_state_control(ec, &ectrls[0], chg_type, req->req_cookie); - rc = slapi_send_ldap_search_entry( req->req_pblock, - ec, ectrls, - noattrs?noattrs:attrs, attrsonly ); + sync_create_state_control(ec, &ectrls[0], chg_type, req->req_cookie); + rc = slapi_send_ldap_search_entry( req->req_pblock, + ec, ectrls, + noattrs?noattrs:attrs, attrsonly ); if (rc) { slapi_log_error(SLAPI_LOG_CONNS, SYNC_PLUGIN_SUBSYSTEM, "Error %d sending entry %s\n", diff --git a/ldap/servers/slapd/back-ldbm/dblayer.c b/ldap/servers/slapd/back-ldbm/dblayer.c index f3159a9..2524355 100644 --- a/ldap/servers/slapd/back-ldbm/dblayer.c +++ b/ldap/servers/slapd/back-ldbm/dblayer.c @@ -4805,19 +4805,18 @@ static int checkpoint_threadmain(void *param) for (inst_obj = objset_first_obj(li->li_instance_set); inst_obj; - inst_obj = objset_next_obj(li->li_instance_set, inst_obj)) { + inst_obj = objset_next_obj(li->li_instance_set, inst_obj)) + { inst = (ldbm_instance *)object_get_data(inst_obj); rc = dblayer_get_id2entry(inst->inst_be, &db); - if (!db) { + if (!db || rc ) { continue; } LDAPDebug1Arg(LDAP_DEBUG_BACKLDBM, "compactdb: Compacting DB start: %s\n", inst->inst_name); rc = dblayer_txn_begin(inst->inst_be, NULL, &txn); if (rc) { - LDAPDebug1Arg(LDAP_DEBUG_ANY, - "compactdb: transaction begin failed: %d\n", - rc); + LDAPDebug1Arg(LDAP_DEBUG_ANY, "compactdb: transaction begin failed: %d\n", rc); break; } rc = db->compact(db, txn.back_txn_txn, NULL/*start*/, NULL/*stop*/, @@ -4826,12 +4825,20 @@ static int checkpoint_threadmain(void *param) LDAPDebug(LDAP_DEBUG_ANY, "compactdb: failed to compact %s; db error - %d %s\n", inst->inst_name, rc, db_strerror(rc)); - rc = dblayer_txn_abort(inst->inst_be, &txn); + if((rc = dblayer_txn_abort(inst->inst_be, &txn))){ + LDAPDebug(LDAP_DEBUG_ANY, "compactdb: failed to abort txn (%s) db error - %d %s\n", + inst->inst_name, rc, db_strerror(rc)); + break; + } } else { LDAPDebug2Args(LDAP_DEBUG_BACKLDBM, "compactdb: compact %s - %d pages freed\n", inst->inst_name, c_data.compact_pages_free); - rc = dblayer_txn_commit(inst->inst_be, &txn); + if((rc = dblayer_txn_commit(inst->inst_be, &txn))){ + LDAPDebug(LDAP_DEBUG_ANY, "compactdb: failed to commit txn (%s) db error - %d %s\n", + inst->inst_name, rc, db_strerror(rc)); + break; + } } } time_of_last_comapctdb_completion = current_time(); /* seconds since epoch */ diff --git a/ldap/servers/slapd/pw.c b/ldap/servers/slapd/pw.c index a4d2dc6..7930be9 100644 --- a/ldap/servers/slapd/pw.c +++ b/ldap/servers/slapd/pw.c @@ -2011,28 +2011,33 @@ slapi_pwpolicy_make_response_control (Slapi_PBlock *pb, int seconds, int logins, } rc = ber_printf( ber, "{" ); - if ( seconds >= 0 || logins >= 0 ) { - if ( seconds >= 0 ) { - rc = ber_printf( ber, "t{ti}", LDAP_TAG_PWP_WARNING, - LDAP_TAG_PWP_SECSLEFT, - seconds ); - } - else { - rc = ber_printf( ber, "t{ti}", LDAP_TAG_PWP_WARNING, - LDAP_TAG_PWP_GRCLOGINS, - logins ); + if ( rc != -1){ + if(seconds >= 0 || logins >= 0 ) { + if ( seconds >= 0 ) { + rc = ber_printf( ber, "t{ti}", LDAP_TAG_PWP_WARNING, + LDAP_TAG_PWP_SECSLEFT, + seconds ); + } + else { + rc = ber_printf( ber, "t{ti}", LDAP_TAG_PWP_WARNING, + LDAP_TAG_PWP_GRCLOGINS, + logins ); + } + } + if (rc != -1){ + if ( error >= 0 ) { + rc = ber_printf( ber, "te", LDAP_TAG_PWP_ERROR, error ); + } + if (rc != -1){ + rc = ber_printf( ber, "}" ); + if ( rc != -1 ) + { + rc = ber_flatten( ber, &bvp ); + } + } } } - if ( error >= 0 ) { - rc = ber_printf( ber, "te", LDAP_TAG_PWP_ERROR, error ); - } - rc = ber_printf( ber, "}" ); - if ( rc != -1 ) - { - rc = ber_flatten( ber, &bvp ); - } - ber_free( ber, 1 ); if ( rc != -1 ) @@ -2041,11 +2046,11 @@ slapi_pwpolicy_make_response_control (Slapi_PBlock *pb, int seconds, int logins, new_ctrl.ldctl_oid = LDAP_X_CONTROL_PWPOLICY_RESPONSE; new_ctrl.ldctl_value = *bvp; new_ctrl.ldctl_iscritical = 0; - rc= slapi_pblock_set( pb, SLAPI_ADD_RESCONTROL, &new_ctrl ); + rc = slapi_pblock_set( pb, SLAPI_ADD_RESCONTROL, &new_ctrl ); ber_bvfree(bvp); } - LDAPDebug( LDAP_DEBUG_TRACE, "<= slapi_pwpolicy_make_response_control", 0, 0, 0 ); + LDAPDebug( LDAP_DEBUG_TRACE, "<= slapi_pwpolicy_make_response_control (%d)", rc, 0, 0 ); return (rc==-1?LDAP_OPERATIONS_ERROR:LDAP_SUCCESS); } -- 1.9.3