#49676 Ticket 49675 - Fix coverity issues
Merged 2 years ago by mreynolds. Opened 2 years ago by mreynolds.
mreynolds/389-ds-base coverity  into  master

@@ -73,7 +73,7 @@ 

  static int ruvReplicaCompare(const void *el1, const void *el2);

  static RUVElement *get_ruvelement_from_berval(const struct berval *bval);

  static char *get_replgen_from_berval(const struct berval *bval);

- 

+ static PRInt32 ruv_replica_count_nolock(const RUV *ruv, int lock);

  static const char *const prefix_replicageneration = "{replicageneration}";

  static const char *const prefix_ruvcsn = "{replica "; /* intentionally missing '}' */

  

@@ -1366,22 +1366,30 @@ 

      return rc;

  }

  

- PRInt32

- ruv_replica_count(const RUV *ruv)

+ static PRInt32

+ ruv_replica_count_nolock(const RUV *ruv, int lock)

  {

      if (ruv == NULL)

          return 0;

      else {

          int count;

  

-         slapi_rwlock_rdlock(ruv->lock);

+         if (lock)

+             slapi_rwlock_rdlock(ruv->lock);

          count = dl_get_count(ruv->elements);

-         slapi_rwlock_unlock(ruv->lock);

+         if (lock)

+             slapi_rwlock_unlock(ruv->lock);

  

          return count;

      }

  }

  

+ PRInt32

+ ruv_replica_count(const RUV *ruv)

+ {

+     return ruv_replica_count_nolock(ruv, 1);

+ }

+ 

  /*

   * Extract all the referral URL's from the RUV (but self URL),

   * returning them in an array of strings, that

@@ -1406,7 +1414,7 @@ 

  

      slapi_rwlock_rdlock(ruv->lock);

  

-     n = ruv_replica_count(ruv);

+     n = ruv_replica_count_nolock(ruv, 0);

      if (n > 0) {

          RUVElement *replica;

          int cookie;

@@ -1664,7 +1672,11 @@ 

      slapi_rwlock_wrlock(ruv->lock);

      if (local_rid != prim_rid) {

          repl_ruv = ruvGetReplica(ruv, prim_rid);

-         rc = ruv_update_ruv_element(ruv, repl_ruv, prim_csn, replica_purl, PR_FALSE);

+         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;

@mreynolds, you need to release the lock before returning

And coverity just complained about it too. I'll get this fixed...

+         }

      }

      repl_ruv = ruvGetReplica(ruv, local_rid);

      rc = ruv_update_ruv_element(ruv, repl_ruv, prim_csn, replica_purl, PR_TRUE);

@@ -500,7 +500,7 @@ 

      }

  

      /* Allocate the buffer */

-     filter = slapi_ch_malloc(filterLen);

+     filter = (char *)slapi_ch_calloc(1, filterLen + 1);

      fp = filter;

      max = &filter[filterLen];

  

@@ -359,10 +359,10 @@ 

  /* for the in-core cache of entries */

  struct cache

  {

-     uint64_t c_maxsize;         /* max size in bytes */

+     uint64_t c_maxsize;       /* max size in bytes */

      Slapi_Counter *c_cursize; /* size in bytes */

-     uint64_t c_maxentries;        /* max entries allowed (-1: no limit) */

-     uint64_t c_curentries;        /* current # entries in cache */

+     int64_t c_maxentries;     /* max entries allowed (-1: no limit) */

+     uint64_t c_curentries;    /* current # entries in cache */

      Hashtable *c_dntable;

      Hashtable *c_idtable;

  #ifdef UUIDCACHE_ON

@@ -494,7 +494,7 @@ 

  

  /* initialize the cache */

  int

- cache_init(struct cache *cache, uint64_t maxsize, uint64_t maxentries, int type)

+ cache_init(struct cache *cache, uint64_t maxsize, int64_t maxentries, int type)

  {

      slapi_log_err(SLAPI_LOG_TRACE, "cache_init", "-->\n");

      cache->c_maxsize = maxsize;

@@ -703,7 +703,7 @@ 

  }

  

  void

- cache_set_max_entries(struct cache *cache, long entries)

+ cache_set_max_entries(struct cache *cache, int64_t entries)

  {

      struct backentry *eflush = NULL;

      struct backentry *eflushtemp = NULL;

@@ -742,10 +742,10 @@ 

      return n;

  }

  

- long

+ int64_t

  cache_get_max_entries(struct cache *cache)

  {

-     long n;

+     int64_t n;

  

      cache_lock(cache);

      n = cache->c_maxentries;

@@ -773,7 +773,7 @@ 

   * these u_long *'s to a struct

   */

  void

- cache_get_stats(struct cache *cache, PRUint64 *hits, PRUint64 *tries, uint64_t *nentries, uint64_t *maxentries, uint64_t *size, uint64_t *maxsize)

+ cache_get_stats(struct cache *cache, PRUint64 *hits, PRUint64 *tries, uint64_t *nentries, int64_t *maxentries, uint64_t *size, uint64_t *maxsize)

  {

      cache_lock(cache);

      if (hits)

@@ -1431,7 +1431,7 @@ 

          /* don't add to lru since refcnt = 1 */

          LOG("added entry of size %lu -> total now %lu out of max %lu\n",

              e->ep_size, slapi_counter_get_value(cache->c_cursize), cache->c_maxsize);

-         if (cache->c_maxentries >= 0) {

+         if (cache->c_maxentries > 0) {

              LOG("    total entries %ld out of %ld\n",

                  cache->c_curentries, cache->c_maxentries);

          }

@@ -1838,7 +1838,7 @@ 

          LOG("added entry of size %lu -> total now %lu out of max %lu\n",

              bdn->ep_size, slapi_counter_get_value(cache->c_cursize),

              cache->c_maxsize);

-         if (cache->c_maxentries >= 0) {

+         if (cache->c_maxentries > 0) {

              LOG("    total entries %ld out of %ld\n",

                  cache->c_curentries, cache->c_maxentries);

          }

@@ -1862,7 +1862,7 @@ 

              if (PL_strchr(rdnp, '\\')) {

                  do_dn_norm = 1;

              } else {

-                 while ((++rdnp <= endrdn) && (*rdnp == ' ') && (*rdnp == '\t'))

+                 while ((++rdnp <= endrdn) && (*rdnp == ' ' || *rdnp == '\t'))

                      ;

                  /* DN contains an RDN <type>="<value>" ? */

                  if ((rdnp != endrdn) && ('"' == *rdnp) && ('"' == *endrdn)) {

@@ -48,7 +48,8 @@ 

      struct berval *vals[2];

      char buf[BUFSIZ];

      uint64_t hits, tries;

-     uint64_t nentries, maxentries;

+     uint64_t nentries;

+     int64_t maxentries;

      uint64_t size, maxsize;

      /* NPCTE fix for bugid 544365, esc 0. <P.R> <04-Jul-2001> */

      struct stat astat;

@@ -100,7 +101,7 @@ 

      MSET("maxEntryCacheSize");

      sprintf(buf, "%" PRIu64, nentries);

      MSET("currentEntryCacheCount");

-     sprintf(buf, "%" PRIu64, maxentries);

+     sprintf(buf, "%" PRId64, maxentries);

      MSET("maxEntryCacheCount");

  

      if (entryrdn_get_switch()) {

@@ -119,7 +120,7 @@ 

          MSET("maxDnCacheSize");

          sprintf(buf, "%" PRIu64, nentries);

          MSET("currentDnCacheCount");

-         sprintf(buf, "%" PRIu64, maxentries);

+         sprintf(buf, "%" PRId64, maxentries);

          MSET("maxDnCacheCount");

      }

  

@@ -32,14 +32,14 @@ 

  /*

   * cache.c

   */

- int cache_init(struct cache *cache, uint64_t maxsize, uint64_t maxentries, int type);

+ int cache_init(struct cache *cache, uint64_t maxsize, int64_t maxentries, int type);

  void cache_clear(struct cache *cache, int type);

  void cache_destroy_please(struct cache *cache, int type);

  void cache_set_max_size(struct cache *cache, uint64_t bytes, int type);

  void cache_set_max_entries(struct cache *cache, long entries);

  size_t cache_get_max_size(struct cache *cache);

  long cache_get_max_entries(struct cache *cache);

- void cache_get_stats(struct cache *cache, uint64_t *hits, uint64_t *tries, uint64_t *entries, uint64_t *maxentries, uint64_t *size, uint64_t *maxsize);

+ void cache_get_stats(struct cache *cache, uint64_t *hits, uint64_t *tries, uint64_t *entries, int64_t *maxentries, uint64_t *size, uint64_t *maxsize);

  void cache_debug_hash(struct cache *cache, char **out);

  int cache_remove(struct cache *cache, void *e);

  void cache_return(struct cache *cache, void **bep);

@@ -36,9 +36,13 @@ 

          int oldsize = maxbackends;

          maxbackends += BACKEND_GRAB_SIZE;

          backends = (Slapi_Backend **)slapi_ch_realloc((char *)backends, maxbackends * sizeof(Slapi_Backend *));

-         memset(&backends[oldsize], '\0', BACKEND_GRAB_SIZE * sizeof(Slapi_Backend *));

+         for (i = oldsize; i < maxbackends; i++){

+             /* init the new be pointers so we can track empty slots */

+             backends[i] = NULL;

+         }

      }

  

+     /* Find the first open slot */

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

          ;

  

file modified
+4 -2

@@ -1005,8 +1005,10 @@ 

                   * we need to open and fsync the dir to make the rename stick.

                   */

                  int fp_configdir = open(pdse->dse_configdir, O_PATH | O_DIRECTORY);

-                 fsync(fp_configdir);

-                 close(fp_configdir);

+                 if (fp_configdir != -1) {

+                     fsync(fp_configdir);

+                     close(fp_configdir);

+                 }

              }

          }

          if (fpw.fpw_prfd)

@@ -146,29 +146,34 @@ 

           * that it was done by the root DN. */

          Connection *pb_conn = NULL;

          slapi_pblock_get(pb_orig, SLAPI_CONNECTION, &pb_conn);

-         slapi_pblock_set(pb, SLAPI_CONNECTION, pb_conn);

+         if (pb_conn){

+             slapi_pblock_set(pb, SLAPI_CONNECTION, pb_conn);

+             ret = slapi_modify_internal_pb(pb);

  

-         ret = slapi_modify_internal_pb(pb);

+             /* We now clean up the connection that we copied into the

+              * new pblock.  We want to leave it untouched. */

+             slapi_pblock_set(pb, SLAPI_CONNECTION, NULL);

  

-         /* We now clean up the connection that we copied into the

-          * new pblock.  We want to leave it untouched. */

-         slapi_pblock_set(pb, SLAPI_CONNECTION, NULL);

+             slapi_pblock_get(pb, SLAPI_PLUGIN_INTOP_RESULT, &ret);

  

-         slapi_pblock_get(pb, SLAPI_PLUGIN_INTOP_RESULT, &ret);

- 

-         /* Retrieve and duplicate the response controls since they will be

-          * destroyed along with the pblock used for the internal operation. */

-         slapi_pblock_get(pb, SLAPI_RESCONTROLS, &pb_resp_controls);

-         if (pb_resp_controls) {

-             slapi_add_controls(resp_controls, pb_resp_controls, 1);

-         }

+             /* Retrieve and duplicate the response controls since they will be

+              * destroyed along with the pblock used for the internal operation. */

+             slapi_pblock_get(pb, SLAPI_RESCONTROLS, &pb_resp_controls);

+             if (pb_resp_controls) {

+                 slapi_add_controls(resp_controls, pb_resp_controls, 1);

+             }

  

-         if (ret != LDAP_SUCCESS) {

-             slapi_log_err(SLAPI_LOG_TRACE, "passwd_apply_mods",

-                           "WARNING: passwordPolicy modify error %d on entry '%s'\n",

-                           ret, slapi_sdn_get_dn(sdn));

+             if (ret != LDAP_SUCCESS) {

+                 slapi_log_err(SLAPI_LOG_TRACE, "passwd_apply_mods",

+                               "WARNING: passwordPolicy modify error %d on entry '%s'\n",

+                               ret, slapi_sdn_get_dn(sdn));

+             }

+         } else {

+             ret = -1;

+             slapi_log_err(SLAPI_LOG_ERR, "passwd_apply_mods",

+                           "(%s) Original connection is NULL\n",

+                            slapi_sdn_get_dn(sdn));

          }

- 

          slapi_pblock_destroy(pb);

      }

  

file modified
+1 -1

@@ -2448,7 +2448,7 @@ 

  

      /* Free config data */

  

-     if (!svrcore_setup()) {

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

If there is not token should we still need to call svrcore_setup ?

Also this function (slapd-SSL-client-auth) returns 'rc'.
But if svrcore_setup fails or token is missing it returns 0. It should return a failure and call slapd_SSL_warn

@tbordaz Token is required in that code block so it must not be NULL.

I handled the error condition for svrcore_setup()/token. I need a new pull request to handle this though...

  #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.");

@@ -600,6 +600,7 @@ 

      if (task->task_log_lock == NULL) {

          /* Failed to allocate! Uh Oh! */

          slapi_ch_free((void **)&task);

+         slapi_ch_free_string(&dn);

          slapi_log_err(SLAPI_LOG_ERR, "new_task", "Unable to allocate task lock for: %s\n", rawdn);

          return NULL;

      }

@@ -115,7 +115,7 @@ 

      size_t step = 0;

      size_t current_step = 0;

      size_t max_factors = info->iter / 2048;

-     size_t baseid;

+     size_t baseid = 0;

      void *output;

      /* Give ourselves a unique base id */

      for (size_t i = 0; i < info->tid; i++) {

@@ -188,7 +188,7 @@ 

      size_t cf = 0;

      size_t step = 0;

      size_t max_factors = info->iter / 2048;

-     size_t baseid;

+     size_t baseid = 0;

      /* Give ourselves a unique base id */

      for (size_t i = 0; i < info->tid; i++) {

          baseid += 50000;

@@ -215,7 +215,7 @@ 

      size_t cf = 0;

      size_t step = 0;

      size_t max_factors = info->iter / 2048;

-     size_t baseid;

+     size_t baseid = 0;

      /* Give ourselves a unique base id */

      for (size_t i = 0; i < info->tid; i++) {

          baseid += 50000;

file modified
-13

@@ -29,8 +29,6 @@ 

  static const char retryWarning[] =

  "Warning: Incorrect PIN may result in disabling the token";

  static const char prompt[] = "Enter PIN for";

- static const char nt_retryWarning[] =

- "Warning: You entered an incorrect PIN. Incorrect PIN may result in disabling the token";

  

  struct SVRCOREUserPinObj

  {

@@ -122,14 +120,6 @@ 

    /* If the program is not interactive then return no result */

    if (!tty->interactive) return 0;

  

- #ifdef _WIN32 

-   if (retry) {

-         MessageBox(GetDesktopWindow(), nt_retryWarning,

-                         "Netscape Server", MB_ICONEXCLAMATION | MB_OK);

-   }

-   return NT_PromptForPin(tokenName);

- #else

- 

    if (retry)

      fprintf(stdout, "%s\n", retryWarning);

  

@@ -168,9 +158,6 @@ 

    if (line[0] == 0) return 0;

  

    return strdup(line);

- 

- #endif /* _WIN32 */

- 

  }

  

  /*

Description: Fixed these coverity issues. Some of these fixes are
just to quiet convscan:

16852 Unsigned compared - entrycache_add_int
16848 Unsigned compared - dncache_add_int
16704 Explicit null dereferenced s- lapd_SSL_client_auth
15953 Resource leak - new_task
15583 Out-of-bounds read - create_filter
15445 Unused value - ruv_update_ruv
15442 Argument cannot be negative - dse_write_file_nolock
15223 Double unlock - ruv_get_referrals
15170 Explicit null dereferenced - passwd_apply_mods
15581 Wrong sizeof argument - slapi_be_new
15144 Constant expression result - upgradedn_producer

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

Reviewed by: ?

rebased onto 2772a29aeeb749e3631ccb4062289fe004d2fa37

2 years ago

I've checked basic, replication acceptance, password and TLS test suites. Everything works fine.

Also, the code looks good to me. But I am not familier with all of the parts of the code base.
So I think, it should be also reviewed by other devs. :)

rebased onto 7e56469b0fb7fbf0929d74b15786b22b0cc42969

2 years ago

rebased onto 7a8b5ac

2 years ago

Pull-Request has been merged by mreynolds

2 years ago

@mreynolds, you need to release the lock before returning

If there is not token should we still need to call svrcore_setup ?

Also this function (slapd-SSL-client-auth) returns 'rc'.
But if svrcore_setup fails or token is missing it returns 0. It should return a failure and call slapd_SSL_warn

And coverity just complained about it too. I'll get this fixed...

@tbordaz Token is required in that code block so it must not be NULL.

I handled the error condition for svrcore_setup()/token. I need a new pull request to handle this though...