389-ds-base

Clone
Source Code
GIT

#49910 Ticket 49029 - Internal logging thread data needs to allocate int pointers
Closed 3 years ago by spichugi. Opened 5 years ago by mreynolds.
mreynolds/389-ds-base ticket49029  into  master

file modified
+10 -19 
@@ -520,24 +520,13 @@ 
 

  {
 

      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 @@ 
 

      SIGNAL(SIGPIPE, SIG_IGN);
 

  #endif
 

  

+     slapi_td_init_internal_logging();
 

+ 

      while (1) {
 

          int is_timedout = 0;
 

          time_t curtime = 0;
 
@@ -1534,6 +1525,7 @@ 
 

                            "op_thread received shutdown signal\n");
 

              slapi_pblock_destroy(pb);
 

              g_decr_active_threadcnt();
 

+             slapi_td_free_internal_logging();
 

              return;
 

          }
 

  
@@ -1555,6 +1547,7 @@ 
 

                                "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 @@ 
 

                      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 @@ 
 

              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 @@ 
 

              PR_ExitMonitor(conn->c_mutex);
 

              signal_listner();
 

              slapi_pblock_destroy(pb);
 

+             slapi_td_free_internal_logging();
 

              return;
 

          }
 

          /*
 
@@ -1819,12 +1815,6 @@ 
 

           * 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 @@ 
 

              PR_ExitMonitor(conn->c_mutex);
 

          }
 

      } /* while (1) */
 

+     slapi_td_free_internal_logging();
 

  }
 

  

  /* thread need to hold conn->c_mutex before calling this function */

file modified
+3 -6 
@@ -1066,7 +1066,6 @@ 
 

      /* -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 @@ 
 

  

          /* 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 @@ 
 

      vattr_cleanup();
 

      sasl_map_done();
 

  cleanup:
 

+     slapi_td_free_internal_logging();
 

      compute_terminate();
 

      SSL_ShutdownServerSessionIDCache();
 

      SSL_ClearSessionCache();
 
@@ -1206,6 +1202,7 @@ 
 

      NSS_Shutdown();
 

      main_stop_ns(tp);
 

      PR_Cleanup();
 

+ 

      /*
 

       * Server has stopped, lets force everything to disk: logs
 

       * db, dse.ldif, all of it.

file modified
+3 
@@ -1185,6 +1185,9 @@ 
 

  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

file modified
+81 -41 
@@ -55,8 +55,6 @@ 
 

  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_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 @@ 
 

  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 @@ 
 

  

      /* 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 @@ 
 

          /* 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 @@ 
 

          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_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);
 

  }
 

  

  /*

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: ?

I've tested this PR with and without ASAN. The ipa-server-install command was successful, no crashes were observed.

rebased onto 9f9f4a9
5 years ago

ack

Pull-Request has been merged by mreynolds
5 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/2969

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
3 years ago
Metadata
None
No Tags
Changes Summary 4
+10 -19
file changed
ldap/servers/slapd/connection.c
+3 -6
file changed
ldap/servers/slapd/main.c
+3 -0
file changed
ldap/servers/slapd/slapi-private.h
+81 -41
file changed
ldap/servers/slapd/thread_data.c
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.