389-ds-base

Clone
Source Code
GIT

#49729 Ticket 49675 - Revise coverity fix
Closed 3 years ago by spichugi. Opened 5 years ago by mreynolds.
mreynolds/389-ds-base coverity  into  master

file modified
+2 -2 
@@ -550,7 +550,7 @@ 
 

          }
 

  

          /* Build the new list */
 

-         for (i = 0; theConfig.groupattrs && theConfig.groupattrs[i]; i++) {
 

+         for (i = 0; theConfig.group_slapiattrs && theConfig.groupattrs && theConfig.groupattrs[i]; i++) {
 

              theConfig.group_slapiattrs[i] = slapi_attr_new();
 

              slapi_attr_init(theConfig.group_slapiattrs[i], theConfig.groupattrs[i]);
 

          }
 
@@ -731,7 +731,7 @@ 
 

              }
 

  

              /* Copy the attributes. */
 

-             for (i = 0; src->group_slapiattrs && src->group_slapiattrs[i]; i++) {
 

+             for (i = 0; dest->group_slapiattrs && src->group_slapiattrs && src->group_slapiattrs[i]; i++) {
 

                  dest->group_slapiattrs[i] = slapi_attr_dup(src->group_slapiattrs[i]);
 

              }

file modified
+4 -3 
@@ -1673,9 +1673,10 @@ 
 

      if (local_rid != prim_rid) {
 

          repl_ruv = ruvGetReplica(ruv, prim_rid);
 

          if ((rc = ruv_update_ruv_element(ruv, repl_ruv, prim_csn, replica_purl, PR_FALSE))) {
 

-            slapi_log_err(SLAPI_LOG_REPL, repl_plugin_name,
 

-                          "ruv_update_ruv - failed to update primary ruv, error (%d)", rc);
 

-            return rc;
 

+             slapi_rwlock_unlock(ruv->lock);
 

+             slapi_log_err(SLAPI_LOG_REPL, repl_plugin_name,
 

+                           "ruv_update_ruv - failed to update primary ruv, error (%d)", rc);
 

+             return rc;
 

          }
 

      }
 

      repl_ruv = ruvGetReplica(ruv, local_rid);

file modified
+11 -11 
@@ -454,10 +454,10 @@ 
 

  static char *
 

  create_filter(const char **attributes, const struct berval *value, const char *requiredObjectClass)
 

  {
 

-     char *filter;
 

+     char *filter = NULL;
 

      char *fp;
 

      char *max;
 

-     int *attrLen;
 

+     int *attrLen = NULL;
 

      int totalAttrLen = 0;
 

      int attrCount = 0;
 

      int valueLen;
 
@@ -487,9 +487,10 @@ 
 

          totalAttrLen += 3;
 

      }
 

  

-     if (ldap_quote_filter_value(value->bv_val,
 

-                                 value->bv_len, 0, 0, &valueLen))
 

-         return 0;
 

+     if (ldap_quote_filter_value(value->bv_val, value->bv_len, 0, 0, &valueLen)) {
 

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

+         return filter;
 

+     }
 

  

      if (requiredObjectClass) {
 

          classLen = strlen(requiredObjectClass);
 
@@ -523,9 +524,9 @@ 
 

          *fp++ = '=';
 

  

          /* Place value in filter */
 

-         if (ldap_quote_filter_value(value->bv_val, value->bv_len,
 

-                                     fp, max - fp, &valueLen)) {
 

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

+         if (ldap_quote_filter_value(value->bv_val, value->bv_len, fp, max - fp, &valueLen)) {
 

+             slapi_ch_free_string(&filter);
 

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

              return 0;
 

          }
 

          fp += valueLen;
 
@@ -545,9 +546,8 @@ 
 

              *fp++ = '=';
 

  

              /* Place value in filter */
 

-             if (ldap_quote_filter_value(value->bv_val, value->bv_len,
 

-                                         fp, max - fp, &valueLen)) {
 

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

+             if (ldap_quote_filter_value(value->bv_val, value->bv_len, fp, max - fp, &valueLen)) {
 

+                 slapi_ch_free_string(&filter);
 

                  slapi_ch_free((void **)&attrLen);
 

                  return 0;
 

              }

file modified
+4 
@@ -5656,6 +5656,7 @@ 
 

                                                inst_dir, MAXPATHLEN);
 

          if (!inst_dirp || !*inst_dirp) {
 

              slapi_log_err(SLAPI_LOG_ERR, "dblayer_copy_directory", "Instance dir is NULL.\n");
 

+             slapi_ch_free_string(&inst_dirp);
 

              return return_value;
 

          }
 

          len = strlen(inst_dirp);
 
@@ -5969,6 +5970,7 @@ 
 

                      slapi_task_log_notice(task,
 

                                            "Backup: Instance dir is empty\n");
 

                  }
 

+                 slapi_ch_free_string(&inst_dirp);
 

                  return_value = -1;
 

                  goto bail;
 

              }
 
@@ -7090,6 +7092,7 @@ 
 

      inst_dirp = dblayer_get_full_inst_dir(inst->inst_li, inst,
 

                                            inst_dir, MAXPATHLEN);
 

      if (!inst_dirp || !*inst_dirp) {
 

+         slapi_ch_free_string(&inst_dirp);
 

          rval = -1;
 

          goto done;
 

      }
 
@@ -7141,6 +7144,7 @@ 
 

      if (NULL == inst_dirp || '\0' == *inst_dirp) {
 

          slapi_log_err(SLAPI_LOG_ERR,
 

                        "dblayer_update_db_ext", "Instance dir is NULL\n");
 

+         slapi_ch_free_string(&inst_dirp);
 

          return -1; /* non zero */
 

      }
 

      for (a = (struct attrinfo *)avl_getfirst(inst->inst_attrs);

file modified
+1 -1 
@@ -1639,7 +1639,7 @@ 
 

  

      dblayer_release_id2entry(be, db);
 

  

-     if (fd > STDERR_FILENO) {
 

+     if (fd >= 0) {
 

          close(fd);
 

      }

file modified
+7 -7 
@@ -42,18 +42,18 @@ 
 

          }
 

      }
 

  

-     /* Find the first open slot */
 

-     for (i = 0; ((i < maxbackends) && (backends[i])); i++)
 

-         ;
 

- 

-     PR_ASSERT(i < maxbackends);
 

  

      be = (Slapi_Backend *)slapi_ch_calloc(1, sizeof(Slapi_Backend));
 

      be->be_lock = slapi_new_rwlock();
 

      be_init(be, type, name, isprivate, logchanges, defsize, deftime);
 

  

-     backends[i] = be;
 

-     nbackends++;
 

+     for (size_t i = 0; i < maxbackends; i++) {
 

+         if (backends[i] == NULL) {
 

+             backends[i] = be;
 

+             nbackends++;
 

+             break;
 

+         }
 

+     }
 

  

      slapi_log_err(SLAPI_LOG_TRACE, "slapi_be_new",
 

                    "Added new backend name [%s] type [%s] nbackends [%d]\n",

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

      if (old_pw) {
 

          /* we have a password to replace with the oldest one in the history. */
 

          if (!values_replace || !vacnt) { /* This is the first one to store */
 

+             slapi_ch_array_free(values_replace);
 

              values_replace = (char **)slapi_ch_calloc(2, sizeof(char *));
 

          }
 

      } else {

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

  

      /* Free config data */
 

  

-     if (!svrcore_setup() && token != NULL) {
 

+     if (token && !svrcore_setup()) {
 

  #ifdef WITH_SYSTEMD
 

          slapd_SSL_warn("Sending pin request to SVRCore. You may need to run "
 

                         "systemd-tty-ask-password-agent to provide the password.");
 
@@ -2460,6 +2460,11 @@ 
 

                  }
 

              }
 

          }
 

+     } else {
 

+         if (token == NULL) {
 

+             slapd_SSL_warn("slapd_SSL_client_auth - certificate token was not found\n");
 

+         }
 

+         rc = -1;
 

      }
 

  

      slapi_ch_free_string(&token);

Description: Fix issues with last coverity patch: missing unlock, and a
return code was needed.

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

Reviewed by: ?

rebased onto 7da16c4e65871bc7e5b03120066b49f5fe54adee
5 years ago

rebased onto 363362584c34d58bbf1052620e920f069fdc4fab
5 years ago

rebased onto 0597c66329f69f9b4fd08159cc2ca8b7d842bffd
5 years ago

I think there is the same issue there https://pagure.io/fork/mreynolds/389-ds-base/blob/coverity/f/ldap/servers/plugins/uiduniq/uid.c#_492

You may also do a 'goto common' at each place it use to 'return'

I am not sure I fully understand the need of that change.
However I think the backends[] will grow by BACKEND_GRAB_SIZE+1 on each realloc but only the BACEKND_GRAB_SIZE slots will be used
For example if initial size backends[11] but maxbackend is 10, then on next realloc it will be backends[22] but maxbackends will be 20...

It should not create any issue (except small lose of space).

I was also wondering if in https://pagure.io/fork/mreynolds/389-ds-base/blob/coverity/f/ldap/servers/slapd/ssl.c#_2379

it could be
if (token != NULL && !svrcore_setup() ) {
..

If there is not Token, is it useful to call svrcore_setup ?

@tbordaz yes this was an odd one. The problem happens when we look for extra/empty slots further down in the code, there a for loop to get the index (via "i") and if it was at the last slot, "i" can be higher than maxbackends. So we need that one extra slot in that corner case.

Also there should only be one extra slot no matter how many times we increase the maxbackends. Since we don't actually increment maxbackends by one, the realloc each time should only be one slot larger (it should not compound each time)

rebased onto 4a61f1bbed561f9f115e10c1ef38b63db0993153
5 years ago

I think we could fix the issue by ensuring we use a slot inside the allocated array, I have attached a proposal to the ticket

rebased onto 89d1946a0a334105e95540d6362e00344c30f7c8
5 years ago

It looks good to me. You have my ACK. Ludwig will give his double ACK ;)

AckAck

rebased onto a37c8ec02427dacbca308339ba5ccef371b0c253
5 years ago

rebased onto 1b7198a
5 years ago

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/2788

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 8
+2 -2
file changed
ldap/servers/plugins/memberof/memberof_config.c
+4 -3
file changed
ldap/servers/plugins/replication/repl5_ruv.c
+11 -11
file changed
ldap/servers/plugins/uiduniq/uid.c
+4 -0
file changed
ldap/servers/slapd/back-ldbm/dblayer.c
+1 -1
file changed
ldap/servers/slapd/back-ldbm/ldif2ldbm.c
+7 -7
file changed
ldap/servers/slapd/backend_manager.c
+1 -0
file changed
ldap/servers/slapd/pw.c
+6 -1
file changed
ldap/servers/slapd/ssl.c
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.