From 9f9f4a9d82a1342facf34956d1355fa76fc97313 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Aug 16 2018 14:18:44 +0000 Subject: Ticket 49029 - Internal logging thread data needs to allocate int pointers Bug Description: The original version of the fix incorrectly used stack pointers to update the thread data. These pointers would go out of scope and could cause a crash when updated. Fix Description: Allocate the integer pointers at initalization time for each worker thread including main() thread. Also cleaned up other areas of this code/feature. Passed ASAN tests. https://pagure.io/389-ds-base/issue/49029 Reviewed by: vashirov & lkrispen(Thanks!!) --- diff --git a/ldap/servers/slapd/connection.c b/ldap/servers/slapd/connection.c index 5bc5217..b1f6f20 100644 --- a/ldap/servers/slapd/connection.c +++ b/ldap/servers/slapd/connection.c @@ -520,24 +520,13 @@ connection_dispatch_operation(Connection *conn, Operation *op, Slapi_PBlock *pb) { int minssf = config_get_minssf(); int minssf_exclude_rootdse = 0; - uint64_t td_conn_id; - int32_t td_op_id; - int32_t td_internal_op = 0; - int32_t td_internal_nested_count = 0; - int32_t td_internal_nested_state = 0; #ifdef TCP_CORK int enable_nagle = config_get_nagle(); int pop_cork = 0; #endif - /* Set the connid and op_id to be used by intenral op logging */ - td_conn_id = conn->c_connid; - td_op_id = op->o_opid; - slapi_td_set_val(SLAPI_TD_CONN_ID,(void *)&td_conn_id); - slapi_td_set_val(SLAPI_TD_OP_ID,(void *)&td_op_id); - slapi_td_set_val(SLAPI_TD_OP_INTERNAL_ID, (void *)&td_internal_op); - slapi_td_set_val(SLAPI_TD_OP_NESTED_COUNT, (void *)&td_internal_nested_count); - slapi_td_set_val(SLAPI_TD_OP_NESTED_STATE, (void *)&td_internal_nested_state); + /* Set the connid and op_id to be used by internal op logging */ + slapi_td_reset_internal_logging(conn->c_connid, op->o_opid); /* Get the effective key length now since the first SSL handshake should be complete */ connection_set_ssl_ssf(conn); @@ -1525,6 +1514,8 @@ connection_threadmain() SIGNAL(SIGPIPE, SIG_IGN); #endif + slapi_td_init_internal_logging(); + while (1) { int is_timedout = 0; time_t curtime = 0; @@ -1534,6 +1525,7 @@ connection_threadmain() "op_thread received shutdown signal\n"); slapi_pblock_destroy(pb); g_decr_active_threadcnt(); + slapi_td_free_internal_logging(); return; } @@ -1555,6 +1547,7 @@ connection_threadmain() "op_thread received shutdown signal\n"); slapi_pblock_destroy(pb); g_decr_active_threadcnt(); + slapi_td_free_internal_logging(); return; case CONN_FOUND_WORK_TO_DO: /* note - don't need to lock here - connection should only @@ -1567,6 +1560,7 @@ connection_threadmain() slapi_log_err(SLAPI_LOG_ERR, "connection_threadmain", "pb_conn is NULL\n"); slapi_pblock_destroy(pb); g_decr_active_threadcnt(); + slapi_td_free_internal_logging(); return; } @@ -1633,6 +1627,7 @@ connection_threadmain() slapi_log_err(SLAPI_LOG_ERR, "connection_threadmain", "NULL param: conn (0x%p) op (0x%p)\n", conn, op); slapi_pblock_destroy(pb); g_decr_active_threadcnt(); + slapi_td_free_internal_logging(); return; } maxthreads = config_get_maxthreadsperconn(); @@ -1811,6 +1806,7 @@ connection_threadmain() PR_ExitMonitor(conn->c_mutex); signal_listner(); slapi_pblock_destroy(pb); + slapi_td_free_internal_logging(); return; } /* @@ -1819,12 +1815,6 @@ connection_threadmain() * threads devoted to this connection, and see if * there's more work to do right now on this conn. */ - int32_t def_val = 0; - slapi_td_set_val(SLAPI_TD_CONN_ID, NULL); - slapi_td_set_val(SLAPI_TD_OP_ID, NULL); - slapi_td_set_val(SLAPI_TD_OP_INTERNAL_ID, (void *)&def_val); - slapi_td_set_val(SLAPI_TD_OP_NESTED_COUNT, (void *)&def_val); - slapi_td_set_val(SLAPI_TD_OP_NESTED_STATE, (void *)&def_val); /* number of ops on this connection */ PR_AtomicIncrement(&conn->c_opscompleted); @@ -1906,6 +1896,7 @@ connection_threadmain() PR_ExitMonitor(conn->c_mutex); } } /* while (1) */ + slapi_td_free_internal_logging(); } /* thread need to hold conn->c_mutex before calling this function */ diff --git a/ldap/servers/slapd/main.c b/ldap/servers/slapd/main.c index cb43b9d..d3bf5f6 100644 --- a/ldap/servers/slapd/main.c +++ b/ldap/servers/slapd/main.c @@ -1066,7 +1066,6 @@ main(int argc, char **argv) /* -sduloutre: compute_init() and entry_computed_attr_init() moved up */ if (mcfg.slapd_exemode != SLAPD_EXEMODE_REFERRAL) { - int32_t init_val = 0; int rc; Slapi_DN *sdn; @@ -1115,11 +1114,7 @@ main(int argc, char **argv) /* init the thread data indexes - need to initialize internal logging TD here for bootstrap startup */ slapi_td_init(); - slapi_td_set_val(SLAPI_TD_CONN_ID, NULL); - slapi_td_set_val(SLAPI_TD_OP_ID, NULL); - slapi_td_set_val(SLAPI_TD_OP_INTERNAL_ID, (void *)&init_val); - slapi_td_set_val(SLAPI_TD_OP_NESTED_COUNT, (void *)&init_val); - slapi_td_set_val(SLAPI_TD_OP_NESTED_STATE, (void *)&init_val); + slapi_td_init_internal_logging(); /* * Initialize password storage in entry extension. @@ -1199,6 +1194,7 @@ main(int argc, char **argv) vattr_cleanup(); sasl_map_done(); cleanup: + slapi_td_free_internal_logging(); compute_terminate(); SSL_ShutdownServerSessionIDCache(); SSL_ClearSessionCache(); @@ -1206,6 +1202,7 @@ cleanup: NSS_Shutdown(); main_stop_ns(tp); PR_Cleanup(); + /* * Server has stopped, lets force everything to disk: logs * db, dse.ldif, all of it. diff --git a/ldap/servers/slapd/slapi-private.h b/ldap/servers/slapd/slapi-private.h index 8fa62b4..837ca8e 100644 --- a/ldap/servers/slapd/slapi-private.h +++ b/ldap/servers/slapd/slapi-private.h @@ -1185,6 +1185,9 @@ int slapi_td_set_plugin_locked(void); int slapi_td_set_plugin_unlocked(void); void slapi_td_internal_op_start(void); void slapi_td_internal_op_finish(void); +void slapi_td_init_internal_logging(void); +void slapi_td_reset_internal_logging(uint64_t conn_id, int32_t op_id); +void slapi_td_free_internal_logging(void); /* Thread Local Storage Index Types - thread_data.c */ #define SLAPI_TD_REQUESTOR_DN 1 diff --git a/ldap/servers/slapd/thread_data.c b/ldap/servers/slapd/thread_data.c index 5c89efe..951457b 100644 --- a/ldap/servers/slapd/thread_data.c +++ b/ldap/servers/slapd/thread_data.c @@ -55,8 +55,6 @@ static PRUintn td_op_internal_nested_count; int slapi_td_init(void) { - int32_t init_val = 0; - if (PR_NewThreadPrivateIndex(&td_requestor_dn, td_dn_destructor) == PR_FAILURE) { slapi_log_err(SLAPI_LOG_CRIT, "slapi_td_init", "Failed it create private thread index for td_requestor_dn/td_dn_destructor\n"); return PR_FAILURE; @@ -71,36 +69,33 @@ slapi_td_init(void) slapi_log_err(SLAPI_LOG_CRIT, "slapi_td_init", "Failed it create private thread index for td_conn_id\n"); return PR_FAILURE; } - slapi_td_set_val(SLAPI_TD_CONN_ID, (void *)&init_val); if(PR_NewThreadPrivateIndex(&td_op_id, NULL) == PR_FAILURE){ slapi_log_err(SLAPI_LOG_CRIT, "slapi_td_init", "Failed it create private thread index for td_op_id\n"); return PR_FAILURE; } - slapi_td_set_val(SLAPI_TD_OP_ID, (void *)&init_val); if(PR_NewThreadPrivateIndex(&td_op_internal_id, NULL) == PR_FAILURE){ slapi_log_err(SLAPI_LOG_CRIT, "slapi_td_init", "Failed it create private thread index for td_op_internal_id\n"); return PR_FAILURE; } - slapi_td_set_val(SLAPI_TD_OP_INTERNAL_ID, (void *)&init_val); if(PR_NewThreadPrivateIndex(&td_op_internal_nested_count, NULL) == PR_FAILURE){ slapi_log_err(SLAPI_LOG_CRIT, "slapi_td_init", "Failed it create private thread index for td_op_internal_nested_count\n"); return PR_FAILURE; } - slapi_td_set_val(SLAPI_TD_OP_NESTED_COUNT, (void *)&init_val); + if(PR_NewThreadPrivateIndex(&td_op_internal_nested_state, NULL) == PR_FAILURE){ slapi_log_err(SLAPI_LOG_CRIT, "slapi_td_init", "Failed it create private thread index for td_op_internal_nested_state\n"); return PR_FAILURE; } - slapi_td_set_val(SLAPI_TD_OP_NESTED_STATE, (void *)&init_val); - return PR_SUCCESS; } + + /* * Caller needs to cast value to (void *) */ @@ -307,7 +302,6 @@ slapi_td_get_dn(char **value) void slapi_td_internal_op_start(void) { - int32_t initial_count = 1; int32_t *id_count_ptr = NULL; int32_t *nested_state_ptr = NULL; int32_t *nested_count_ptr = NULL; @@ -321,11 +315,7 @@ slapi_td_internal_op_start(void) /* increment the internal op id counter */ slapi_td_get_val(SLAPI_TD_OP_INTERNAL_ID, (void **)&id_count_ptr); - if (id_count_ptr == NULL){ - id_count_ptr = &initial_count; - } else { - (*id_count_ptr)++; - } + (*id_count_ptr)++; /* * Bump the nested count so we can maintain our counts after plugins call @@ -339,7 +329,6 @@ slapi_td_internal_op_start(void) /* We are now nested, mark it as so */ slapi_td_get_val(SLAPI_TD_OP_NESTED_STATE, (void **)&nested_state_ptr); *nested_state_ptr = NESTED; - slapi_td_set_val(SLAPI_TD_OP_NESTED_STATE, (void *)nested_state_ptr); } else if (*nested_count_ptr == 1) { /* * Back to the beginning, but if we were previously nested then the @@ -349,12 +338,9 @@ slapi_td_internal_op_start(void) if (*nested_state_ptr == UNNESTED){ /* We were nested but anymore, need to bump the internal id count again */ *nested_state_ptr = NOTNESTED; /* reset nested state */ - slapi_td_set_val(SLAPI_TD_OP_NESTED_STATE, (void *)nested_state_ptr); (*id_count_ptr)++; } } - slapi_td_set_val(SLAPI_TD_OP_NESTED_COUNT, (void *)nested_count_ptr); - slapi_td_set_val(SLAPI_TD_OP_INTERNAL_ID, (void *)id_count_ptr); } /* @@ -378,31 +364,85 @@ slapi_td_internal_op_finish(void) } slapi_td_get_val(SLAPI_TD_OP_NESTED_COUNT, (void **)&nested_count_ptr); - if (nested_count_ptr){ - if ( *nested_count_ptr > 1 ){ - /* Nested op just finished, decr op id */ - slapi_td_get_val(SLAPI_TD_OP_INTERNAL_ID, (void **)&id_count_ptr); - if (id_count_ptr){ - (*id_count_ptr)--; - slapi_td_set_val(SLAPI_TD_OP_INTERNAL_ID, (void *)id_count_ptr); - } - if ( (*nested_count_ptr - 1) == 1 ){ - /* - * Okay we are back to the beginning, We were nested but not - * anymore. So when we start the next internal op on this - * conn we need to double increment the internal op id to - * maintain the correct op id sequence. Set the nested state - * to "unnested". - */ - slapi_td_get_val(SLAPI_TD_OP_NESTED_STATE, (void **)&nested_state_ptr); - (*nested_state_ptr) = UNNESTED; - slapi_td_set_val(SLAPI_TD_OP_NESTED_STATE, (void *)nested_state_ptr); - } + if ( *nested_count_ptr > 1 ){ + /* Nested op just finished, decr op id */ + slapi_td_get_val(SLAPI_TD_OP_INTERNAL_ID, (void **)&id_count_ptr); + (*id_count_ptr)--; + + if ( (*nested_count_ptr - 1) == 1 ){ + /* + * Okay we are back to the beginning, We were nested but not + * anymore. So when we start the next internal op on this + * conn we need to double increment the internal op id to + * maintain the correct op id sequence. Set the nested state + * to "unnested". + */ + slapi_td_get_val(SLAPI_TD_OP_NESTED_STATE, (void **)&nested_state_ptr); + *nested_state_ptr = UNNESTED; } - /* decrement nested count */ - (*nested_count_ptr)--; - slapi_td_set_val(SLAPI_TD_OP_NESTED_COUNT, (void *)nested_count_ptr); } + /* decrement nested count */ + (*nested_count_ptr)--; +} + +void +slapi_td_init_internal_logging(void) +{ + uint64_t *conn_id = (uint64_t *)slapi_ch_calloc(1, sizeof(uint64_t)); + int32_t *op_id = (int32_t *)slapi_ch_calloc(1, sizeof(int32_t)); + int32_t *internal_op_id = (int32_t *)slapi_ch_calloc(1, sizeof(int32_t)); + int32_t *nested_count = (int32_t *)slapi_ch_calloc(1, sizeof(int32_t)); + int32_t *nested_state = (int32_t *)slapi_ch_calloc(1, sizeof(int32_t)); + + slapi_td_set_val(SLAPI_TD_CONN_ID, (void *)conn_id); + slapi_td_set_val(SLAPI_TD_OP_ID, (void *)op_id); + slapi_td_set_val(SLAPI_TD_OP_INTERNAL_ID, (void *)internal_op_id); + slapi_td_set_val(SLAPI_TD_OP_NESTED_COUNT, (void *)nested_count); + slapi_td_set_val(SLAPI_TD_OP_NESTED_STATE, (void *)nested_state); +} + +void +slapi_td_reset_internal_logging(uint64_t new_conn_id, int32_t new_op_id) +{ + uint64_t *conn_id; + int32_t *op_id; + int32_t *internal_op_id; + int32_t *nested_count; + int32_t *nested_state; + + slapi_td_get_val(SLAPI_TD_CONN_ID, (void **)&conn_id); + slapi_td_get_val(SLAPI_TD_OP_ID, (void **)&op_id); + slapi_td_get_val(SLAPI_TD_OP_INTERNAL_ID, (void **)&internal_op_id); + slapi_td_get_val(SLAPI_TD_OP_NESTED_COUNT, (void **)&nested_count); + slapi_td_get_val(SLAPI_TD_OP_NESTED_STATE, (void **)&nested_state); + + *conn_id = new_conn_id; + *op_id = new_op_id; + *internal_op_id = 0; + *nested_count = 0; + *nested_state = 0; +} + +void +slapi_td_free_internal_logging(void) +{ + uint64_t *conn_id = NULL; + int32_t *val = NULL; + + slapi_td_get_val(SLAPI_TD_CONN_ID, (void **)&conn_id); + slapi_ch_free((void **)&conn_id); + + slapi_td_get_val(SLAPI_TD_OP_ID, (void **)&val); + slapi_ch_free((void **)&val); + + slapi_td_get_val(SLAPI_TD_OP_INTERNAL_ID, (void **)&val); + slapi_ch_free((void **)&val); + + slapi_td_get_val(SLAPI_TD_OP_NESTED_COUNT, (void **)&val); + slapi_ch_free((void **)&val); + + slapi_td_get_val(SLAPI_TD_OP_NESTED_STATE, (void **)&val); + slapi_ch_free((void **)&val); } /*