389-ds-base

Clone
Source Code
GIT

#50710 Ticket 50709: Several memory leaks reported by Valgrind for 389-ds 1.3.9.1-10
Closed 3 years ago by spichugi. Opened 4 years ago by tbordaz.
tbordaz/389-ds-base ticket_50709  into  master

file modified
+33 -18 
@@ -251,6 +251,7 @@ 
 

  {
 

      struct acl_pblock *aclpb = NULL;
 

      PRNetAddr *client_praddr = NULL;
 

+     PRNetAddr *pb_client_praddr = NULL;
 

      char ip_str[256];
 

      int rv = LAS_EVAL_TRUE;
 

  
@@ -262,25 +263,39 @@ 
 

          return LAS_EVAL_FAIL;
 

      }
 

  

-     client_praddr = (PRNetAddr *)slapi_ch_malloc(sizeof(PRNetAddr));
 

-     if (client_praddr == NULL) {
 

-         slapi_log_err(SLAPI_LOG_ERR, plugin_name, "DS_LASIpGetter - Failed to allocate client_praddr\n");
 

-         return (LAS_EVAL_FAIL);
 

-     }
 

+     slapi_pblock_get(aclpb->aclpb_pblock, SLAPI_CONN_CLIENTNETADDR_ACLIP, &pb_client_praddr);
 

+     if (pb_client_praddr == NULL) {
 

  

-     if (slapi_pblock_get(aclpb->aclpb_pblock, SLAPI_CONN_CLIENTNETADDR, client_praddr) != 0) {
 

-         slapi_log_err(SLAPI_LOG_ERR, plugin_name, "DS_LASIpGetter - Could not get client IP.\n");
 

-         slapi_ch_free((void **)&client_praddr);
 

-         return (LAS_EVAL_FAIL);
 

-     }
 

+         client_praddr = (PRNetAddr *) slapi_ch_malloc(sizeof (PRNetAddr));
 

+         if (client_praddr == NULL) {
 

+             slapi_log_err(SLAPI_LOG_ERR, plugin_name, "DS_LASIpGetter - Failed to allocate client_praddr\n");
 

+             return (LAS_EVAL_FAIL);
 

+         }
 

  

-     rv = PListInitProp(subject, 0, ACL_ATTR_IP, (void *)client_praddr, NULL);
 

-     if (rv < 0) {
 

-         slapi_log_err(SLAPI_LOG_ACL, plugin_name, "DS_LASIpGetter - "
 

-                                                   "Couldn't set the client addr property(%d)\n",
 

-                       rv);
 

-         slapi_ch_free((void **)&client_praddr);
 

-         return LAS_EVAL_FAIL;
 

+         if (slapi_pblock_get(aclpb->aclpb_pblock, SLAPI_CONN_CLIENTNETADDR, client_praddr) != 0) {
 

+             slapi_log_err(SLAPI_LOG_ERR, plugin_name, "DS_LASIpGetter - Could not get client IP.\n");
 

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

+             return (LAS_EVAL_FAIL);
 

+         }
 

+ 

+         rv = PListInitProp(subject, 0, ACL_ATTR_IP, (void *) client_praddr, NULL);
 

+         if (rv < 0) {
 

+             slapi_log_err(SLAPI_LOG_ACL, plugin_name, "DS_LASIpGetter - "
 

+                     "Couldn't set the client addr property(%d)\n",
 

+                     rv);
 

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

+             return LAS_EVAL_FAIL;
 

+         }
 

+ 

+     } else {
 

+         client_praddr = pb_client_praddr;
 

+         rv = PListInitProp(subject, 0, ACL_ATTR_IP, (void *) client_praddr, NULL);
 

+         if (rv < 0) {
 

+             slapi_log_err(SLAPI_LOG_ACL, plugin_name, "DS_LASIpGetter - "
 

+                     "Couldn't set the client addr property(%d)\n",
 

+                     rv);
 

+             return LAS_EVAL_FAIL;
 

+         }
 

      }
 

      if (PR_NetAddrToString(client_praddr, ip_str, sizeof(ip_str)) == PR_SUCCESS) {
 

          slapi_log_err(SLAPI_LOG_ACL, plugin_name, "DS_LASIpGetter - "
 
@@ -290,7 +305,7 @@ 
 

          slapi_log_err(SLAPI_LOG_ACL, plugin_name, "DS_LASIpGetter - "
 

                                                    "Returning client ip address 'unknown'\n");
 

      }
 

- 

+     slapi_pblock_set(aclpb->aclpb_pblock, SLAPI_CONN_CLIENTNETADDR_ACLIP, client_praddr);
 

      return LAS_EVAL_TRUE;
 

  }

file modified
+2 
@@ -206,6 +206,7 @@ 
 

      conn->c_isreplication_session = 0;
 

      slapi_ch_free((void **)&conn->cin_addr);
 

      slapi_ch_free((void **)&conn->cin_destaddr);
 

+     slapi_ch_free((void **)&conn->cin_addr_aclip);
 

      slapi_ch_free_string(&conn->c_ipaddr);
 

      if (conn->c_domain != NULL) {
 

          ber_bvecfree(conn->c_domain);
 
@@ -408,6 +409,7 @@ 
 

              str_destip = str_unknown;
 

          }
 

      }
 

+     slapi_ch_free((void **)&conn->cin_addr_aclip);
 

  

      if (!in_referral_mode) {
 

          /* create a sasl connection */

file modified
+16 
@@ -482,6 +482,14 @@ 
 

          }
 

          pthread_mutex_unlock(&(pblock->pb_conn->c_mutex));
 

          break;
 

+ 	case SLAPI_CONN_CLIENTNETADDR_ACLIP:
 

+         if (pblock->pb_conn == NULL) {
 

+             break;
 

+         }
 

+         pthread_mutex_lock(&(pblock->pb_conn->c_mutex));
 

+         (*(PRNetAddr **) value) = pblock->pb_conn->cin_addr_aclip;
 

+         pthread_mutex_unlock(&(pblock->pb_conn->c_mutex));
 

+         break;

Do we need to take the lock since we are not duplicating the value?

I was using the same locking approach as the others connections setting (auth type/method, client/server addr, conn flags...).
The idea behind locking is that serveral ops/event can get/set a given connection setting. I agree that client address should be set only once and I can not imagine concurrency at this point. But it looks simpler to not make exception when get/set connection setting.

Set, frees the setting so a reader may read something that will be freed (in theory) under it.

 
      case SLAPI_CONN_SERVERNETADDR:
 

          if (pblock->pb_conn == NULL) {
 

              memset(value, 0, sizeof(PRNetAddr));
 
@@ -2571,6 +2579,14 @@ 
 

          pblock->pb_conn->c_authtype = slapi_ch_strdup((char *)value);
 

          pthread_mutex_unlock(&(pblock->pb_conn->c_mutex));
 

          break;
 

+ 	case SLAPI_CONN_CLIENTNETADDR_ACLIP:
 

+         if (pblock->pb_conn == NULL) {
 

+             break;
 

+         }
 

+         pthread_mutex_lock(&(pblock->pb_conn->c_mutex));
 

+         slapi_ch_free((void **)&pblock->pb_conn->cin_addr_aclip);
 

+         pblock->pb_conn->cin_addr_aclip = (PRNetAddr *)value;
 

+         pthread_mutex_unlock(&(pblock->pb_conn->c_mutex));
 

      case SLAPI_CONN_IS_REPLICATION_SESSION:
 

          if (pblock->pb_conn == NULL) {
 

              slapi_log_err(SLAPI_LOG_ERR,

file modified
+1 
@@ -1634,6 +1634,7 @@ 
 

      char *c_external_dn;             /* client DN of this SSL session  */
 

      char *c_external_authtype;       /* used for c_external_dn   */
 

      PRNetAddr *cin_addr;             /* address of client on this conn */
 

+     PRNetAddr *cin_addr_aclip;       /* address of client allocated by acl with 'ip' subject */
 

      PRNetAddr *cin_destaddr;         /* address client connected to    */
 

      struct berval **c_domain;        /* DNS names of client            */
 

      Operation *c_ops;                /* list of pending operations      */

file modified
+1 
@@ -6930,6 +6930,7 @@ 
 

  #define SLAPI_CONN_DN                     143
 

  #define SLAPI_CONN_CLIENTNETADDR          850
 

  #define SLAPI_CONN_SERVERNETADDR          851
 

+ #define SLAPI_CONN_CLIENTNETADDR_ACLIP    853
 

  #define SLAPI_CONN_IS_REPLICATION_SESSION 149
 

  #define SLAPI_CONN_IS_SSL_SESSION         747
 

  #define SLAPI_CONN_CERT                   743

Description of the problem:

When evaluating an ACI with 'ip' subject, it adds a PRNetAddr to the subject
property list. When the list is free (acl__done_aclpb) the property is not freed.

Description of the fix:

Add the property to the pblock (SLAPI_CONN_CLIENTNETADDR_ACLIP) so that it
the property is freed with acl pblock.

https://pagure.io/389-ds-base/issue/50709

Reviewed by: ?

Metadata Update from @tbordaz:
- Pull-request tagged with: WIP
4 years ago

Still need to write a testcase. Easy one acl 'ip' + anonymous search 'cn=config'

indentation is off in this code block

Should we being freeing cin_addr_aclip value, if it is set, before overwriting the value?

We'd need to check all the places that do a set of cin_addr_aclip because often with pblok the caller is responsible for the free, not the pblock, and if we free here we could be double free-ing.

I need to update the PR according to the comments. Just a comment about the testing phase.

The leak is reproducible with suites/acl/keywords_tests.py and the PR fixes it.
Basic and acl tests are successful.

rebased onto ee94dfe524bbaafb30fcfba257ddb0b91ba99483
4 years ago

You don't need to check if a pointer is NULL before freeing it. Free() was improved many years ago to do this check for you. Firstyear did have some concerns about this free though. Did you see his comment and double check if it is okay to free it here?

Right I will change this.

Regarding Firstyear concern, cin_addr_aclip is an ACL_ATTR_IP property added in the subject property list during the aci evaluation.
In theory it should be freed when we are done with aci evaluation (acl__done_aclpb). The concern is that at that time only property lists are freed but none of the property. I guess that it is because we do not know which type is the property and if a simple slapi_ch_free is good enough.
So the actual free of the property is postponed until the connection is reset by adding the property directly in the connection.
The only setting is done when we need to retrieve the PRAddrnet for ACI evaluation.

rebased onto f0a0cfcbc522343c347b152740fdedbefae3e90c
4 years ago

I'm assuming that this passes under ASAN/LSAN? My reading of the code doesn't show any major concerns now, @mreynolds do you have anything else?

@firstyear Sorry for the late answer... Indeed it passes successfully with ASAN/LSAN build

In that case I'm happy, provided it passes asan/lsan :)

You have my ack, I'll leave it to @mreynolds to follow up to be sure he's happy too.

Metadata Update from @tbordaz:
- Pull-request untagged with: WIP
4 years ago

rebased onto 0e3380c0c026144e726fa789c451f0ca3b863038
4 years ago

@mreynolds any concern on this PR ?

Do we need to take the lock since we are not duplicating the value?

I was using the same locking approach as the others connections setting (auth type/method, client/server addr, conn flags...).
The idea behind locking is that serveral ops/event can get/set a given connection setting. I agree that client address should be set only once and I can not imagine concurrency at this point. But it looks simpler to not make exception when get/set connection setting.

Set, frees the setting so a reader may read something that will be freed (in theory) under it.

I was using the same locking approach as the others connections setting (auth type/method, client/server addr, conn flags...).
The idea behind locking is that serveral ops/event can get/set a given connection setting. I agree that client address should be set only once and I can not imagine concurrency at this point. But it looks simpler to not make exception when get/set connection setting.
Set, frees the setting so a reader may read something that will be freed (in theory) under it.

Yeah I have no strong objection to it, it was more of an observation. The value could still be freed when another thread is reading it, but if there is no concurrency potential then it's fine not string dupping it.

Regardless, you have my ack!

rebased onto 463d6b7
4 years ago

Commit a446d97 fixes this pull-request

Pull-Request has been merged by tbordaz
4 years ago

Pull-Request has been merged by tbordaz
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/3765

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 5
+33 -18
file changed
ldap/servers/plugins/acl/acllas.c
+2 -0
file changed
ldap/servers/slapd/connection.c
+16 -0
file changed
ldap/servers/slapd/pblock.c
+1 -0
file changed
ldap/servers/slapd/slap.h
+1 -0
file changed
ldap/servers/slapd/slapi-plugin.h
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.