389-ds-base

Clone
Source Code
GIT

#50956 Issue 50955 - Fix memory leaks in chaining plugin
Closed 3 years ago by spichugi. Opened 4 years ago by mreynolds.
mreynolds/389-ds-base issue50955  into  master

file modified
+12 -1 
@@ -39,6 +39,7 @@ 
 

      Slapi_Backend *be;
 

      cb_backend_instance *inst;
 

      cb_backend *cb = cb_get_backend_type();
 

+     char *cookie;
 

      int rc;
 

  

      slapi_pblock_get(pb, SLAPI_BACKEND, &be);
 
@@ -62,8 +63,18 @@ 
 

          slapi_config_remove_callback(SLAPI_OPERATION_ADD, DSE_FLAG_PREOP, cb->pluginDN,
 

                                       LDAP_SCOPE_SUBTREE, CB_CONFIG_INSTANCE_FILTER,
 

                                       cb_config_add_instance_check_callback);
 

-         free_cb_backend(cb);
 

  

+         be = slapi_get_first_backend(&cookie);

Most of it looks good, just curious about this cookie code here? Do you mind commenting to explain what it's doing? 

+         while (be) {
 

+             const char *betype = slapi_be_gettype(be);
 

+             if (strcasecmp(betype, CB_CHAINING_BACKEND_TYPE) == 0) {
 

+                 inst = cb_get_instance(be);
 

+                 cb_instance_free(inst);
 

+             }
 

+             be = slapi_get_next_backend(cookie);
 

+         }
 

+         slapi_ch_free((void **)&cookie);
 

+         free_cb_backend(cb);
 

          return 0;
 

      }

file modified
+14 -5 
@@ -213,7 +213,6 @@ 
 

  void
 

  cb_instance_free(cb_backend_instance *inst)
 

  {
 

- 

      if (inst) {
 

          slapi_rwlock_wrlock(inst->rwl_config_lock);
 

  
@@ -226,6 +225,8 @@ 
 

              cb_close_conn_pool(inst->bind_pool);
 

              slapi_destroy_condvar(inst->bind_pool->conn.conn_list_cv);
 

              slapi_destroy_mutex(inst->bind_pool->conn.conn_list_mutex);
 

+             slapi_ch_free_string(&inst->bind_pool->mech);
 

+             slapi_ch_free_string(&inst->bind_pool->hostname);
 

              slapi_ch_free((void **)&inst->bind_pool);
 

          }
 

  
@@ -235,7 +236,9 @@ 
 

              slapi_ch_free_string(&inst->pool->password);
 

              slapi_ch_free_string(&inst->pool->binddn);
 

              slapi_ch_free_string(&inst->pool->binddn2);
 

+             slapi_ch_free_string(&inst->pool->mech);
 

              slapi_ch_free_string(&inst->pool->url);
 

+             slapi_ch_free_string(&inst->pool->hostname);
 

              slapi_destroy_mutex(inst->pool->conn.conn_list_mutex);
 

              slapi_ch_free((void **)&inst->pool);
 

          }
 
@@ -703,8 +706,10 @@ 
 

  

      if ((rc = slapi_ldap_url_parse(url, &ludp, 0, &secure)) != 0 || !ludp) {
 

          PL_strncpyz(errorbuf, slapi_urlparse_err2string(rc), SLAPI_DSE_RETURNTEXT_SIZE);
 

-         if (CB_CONFIG_PHASE_INITIALIZATION == phase)
 

+         if (CB_CONFIG_PHASE_INITIALIZATION == phase) {
 

+             slapi_ch_free_string(&inst->pool->url);
 

              inst->pool->url = slapi_ch_strdup("");
 

+         }
 

          rc = LDAP_INVALID_SYNTAX;
 

          goto done;
 

      }
 
@@ -747,12 +752,13 @@ 
 

  

          /* Normal case. Extract useful data from */
 

          /* the url and update the configuration  */
 

- 

+         slapi_ch_free_string(&inst->pool->hostname);
 

          if ((ludp->lud_host == NULL) || (strlen(ludp->lud_host) == 0)) {
 

              inst->pool->hostname = get_localhost_DNS();
 

          } else {
 

              inst->pool->hostname = slapi_ch_strdup(ludp->lud_host);
 

          }
 

+         slapi_ch_free_string(&inst->pool->url);
 

          inst->pool->url = slapi_ch_strdup(url);
 

          inst->pool->secure = secure;
 

  
@@ -794,6 +800,7 @@ 
 

  

          inst->bind_pool->port = inst->pool->port;
 

          inst->bind_pool->secure = inst->pool->secure;
 

+         slapi_ch_free_string(&inst->bind_pool->hostname);
 

          inst->bind_pool->hostname = slapi_ch_strdup(inst->pool->hostname);
 

  

          slapi_rwlock_unlock(inst->rwl_config_lock);
 
@@ -839,6 +846,7 @@ 
 

  

          /* normalize and ignore the case */
 

          slapi_ch_free_string(&inst->pool->binddn);
 

+         slapi_ch_free_string(&inst->pool->binddn2);
 

          inst->pool->binddn = slapi_create_dn_string_case("%s", (char *)value);
 

          /* not normalized */
 

          inst->pool->binddn2 = slapi_ch_strdup((char *)value);
 
@@ -902,7 +910,7 @@ 
 

              charray_add(&inst->pool->waste_basket, inst->pool->password);
 

              rc = CB_REOPEN_CONN;
 

          }
 

- 

+         slapi_ch_free_string(&inst->pool->password);
 

          inst->pool->password = slapi_ch_strdup((char *)value);
 

          slapi_rwlock_unlock(inst->rwl_config_lock);
 

      }
 
@@ -1465,12 +1473,13 @@ 
 

              }
 

              rc = CB_REOPEN_CONN;
 

          }
 

- 

+         slapi_ch_free_string(&inst->pool->mech);
 

          if (value && !PL_strcasecmp((char *)value, CB_SIMPLE_BINDMECH)) {
 

              inst->pool->mech = slapi_ch_strdup(LDAP_SASL_SIMPLE);
 

          } else {
 

              inst->pool->mech = slapi_ch_strdup((char *)value);
 

          }
 

+         slapi_ch_free_string(&inst->bind_pool->mech);
 

          inst->bind_pool->mech = slapi_ch_strdup(inst->pool->mech);
 

          slapi_rwlock_unlock(inst->rwl_config_lock);
 

      }

Bug Description:

There are many leaks caused by reinitializing a chaining backend, and there are other leaks caused with initialization allocations are not freed in the plugin's close() function.

Fix Description:

Make sure we free pointers before blindly overwriting them, and make sure we call chaining instance free function for all chaining backends when stopping the plugin.

relates: https://pagure.io/389-ds-base/issue/50955

Most of it looks good, just curious about this cookie code here? Do you mind commenting to explain what it's doing?

Most of it looks good, just curious about this cookie code here? Do you mind commenting to explain what it's doing?

That's how you traverse the backend list. Check other examples of slapi_get_first_backend() and they all use this "cookie".

Okay, I'll give this a more thorough read on Monday (I should be enjoying my weekend :) )

Okay, I'll give this a more thorough read on Monday (I should be enjoying my weekend :) )

Sure, and in case I missed your question/concern, what that code is doing is looping over all the backends trying to find any chaining backends. If it is a chaining backend then we call the chaining instance free function.

The code that uses a "cookie" is just how those functions are designed to walk all the backends.

Okay, I've gone and had another review while looking at the source too, and I think this is good for me :) ack.

rebased onto d0c3763
4 years ago

Pull-Request has been merged by mreynolds
4 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/4009

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 2
+12 -1
file changed
ldap/servers/plugins/chainingdb/cb_close.c
+14 -5
file changed
ldap/servers/plugins/chainingdb/cb_instance.c
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.