Ticket #161 - Review and address latest Coverity issues

https://fedorahosted.org/389/ticket/161
Resolves: Ticket #161
Bug Description: Review and address latest Coverity issues
Reviewed by: nhosoi (Thanks!)
Branch: master
Fix Description:
12486 12485 12484 Resource leak In
linked_attrs_add_backlinks_callback(): Leak of memory or pointers to
system resources
the return -1 left targetsdn, targets, and pb dangling
allocate targetsdn after shutdown check - instead of return -1, goto done and free resources at done:

12481 Resource leak In config_set_default_naming_context(): Leak of
memory or pointers to system resources
free suffix before return

12477 Uninitialized pointer read In index_addordel_entry(): Reads an
uninitialized pointer or its target
use LDBM_PARENTID_STR instead of type

12476 Dereference after null check In string_assertion2keys_ava():
Pointer is checked against null but then dereferenced anyway
check for NULL val

12475 Logically dead code In _entryrdn_insert_key(): Code can never be
reached because of a logical contradiction
get rid of dead code

12448 Time of check time of use In INTdir_create_all(): A check occurs
on a file's attributes before the file is used in a privileged
operation, but things may have changed
get rid of unused code

12447-12444 Time of check time of use
use open() to open the file, then use the functions that take an fd to
further test or access the file - this prevents someone changing the file
between functions that only use the filename

12434-12425 Copy into fixed size buffer
use strncpy or snprintf and make sure the string is null terminated

Platforms tested: RHEL6 x86_64, Fedora 16
Flag Day: no
Doc impact: no

include/base/file.h

@@ -86,8 +86,6 @@ NSAPI_PUBLIC SYS_DIRENT *INTdir_read(SYS_DIR ds);
 NSAPI_PUBLIC void INTdir_close(SYS_DIR ds);
 #endif /* XP_WIN32 */
 
-NSAPI_PUBLIC int INTdir_create_all(char *dir);
-
 /* --- OBSOLETE ----------------------------------------------------------
  * The following macros/functions are obsolete and are only maintained for
  * compatibility.  Do not use them. 11-19-96
@@ -131,7 +129,6 @@ NSPR_END_EXTERN_C
 #define dir_read INTdir_read
 #define dir_close INTdir_close
 #endif /* XP_WIN32 */
-#define dir_create_all INTdir_create_all
 
 /* Obsolete */
 #ifdef XP_WIN32

ldap/servers/plugins/linkedattrs/fixup_task.c

@@ -369,12 +369,14 @@ linked_attrs_add_backlinks_callback(Slapi_Entry *e, void *callback_data)
     for (i = 0; targets && targets[i]; ++i) {
         char *targetdn = (char *)targets[i];
         int perform_update = 0;
-        Slapi_DN *targetsdn = slapi_sdn_new_normdn_byref(targetdn);
+        Slapi_DN *targetsdn = NULL;
 
         if (g_get_shutdown()) {
-            return -1;
+            rc = -1;
+            goto done;
         }
 
+        targetsdn = slapi_sdn_new_normdn_byref(targetdn);
         if (config->scope) {
             /* Check if the target is within the scope. */
             perform_update = slapi_dn_issuffix(targetdn, config->scope);
@@ -408,6 +410,7 @@ linked_attrs_add_backlinks_callback(Slapi_Entry *e, void *callback_data)
         slapi_sdn_free(&targetsdn);
     }
 
+done:
     slapi_ch_array_free(targets);
     slapi_pblock_destroy(pb);
 

ldap/servers/plugins/syntaxes/string.c

@@ -678,8 +678,8 @@ string_assertion2keys_ava(
         break;
 	case LDAP_FILTER_EQUALITY:
 		(*ivals) = (Slapi_Value **) slapi_ch_malloc( 2 * sizeof(Slapi_Value *) );
-		(*ivals)[0] = slapi_value_dup( val );
-		if (!(flags & SLAPI_ATTR_FLAG_NORMALIZED)) {
+		(*ivals)[0] = val ? slapi_value_dup( val ) : NULL;
+		if (val && !(flags & SLAPI_ATTR_FLAG_NORMALIZED)) {
 			/* 3rd arg: 1 - trim leading blanks */
 			value_normalize_ext( (*ivals)[0]->bv.bv_val, syntax, 1, &alt );
 			if (alt) {

ldap/servers/slapd/back-ldbm/index.c

@@ -379,7 +379,7 @@ index_addordel_entry(
     back_txn		*txn
 )
 {
-    char		*type;
+    char		*type = NULL;
     Slapi_Value	**svals;
     int		rc, result;
     Slapi_Attr		*attr;
@@ -430,7 +430,7 @@ index_addordel_entry(
             slapi_entry_attr_find(e->ep_entry, LDBM_PARENTID_STR, &attr);
             if (attr) {
                 svals = attr_get_present_values(attr);
-                result = index_addordel_values_sv(be, type, svals, NULL,
+                result = index_addordel_values_sv(be, LDBM_PARENTID_STR, svals, NULL,
                                                   e->ep_id, flags, txn);
                 if ( result != 0 ) {
                     ldbm_nasty(errmsg, 1020, result);

ldap/servers/slapd/back-ldbm/ldbm_entryrdn.c

@@ -2278,6 +2278,7 @@ _entryrdn_insert_key(backend *be,
                     goto bail;
                     /* done */
                 } else {
+                    ID currid = 0;
                     rc = _entryrdn_get_tombstone_elem(cursor, tmpsrdn, &key,
                                                           childnrdn, &tmpelem);
                     if (rc || (NULL == tmpelem)) {
@@ -2296,41 +2297,14 @@ _entryrdn_insert_key(backend *be,
                         goto bail;
                     }
                     /* Node is a tombstone. */
-                    ID currid = 0;
                     slapi_ch_free((void **)&elem);
                     elem = tmpelem;
                     currid = id_stored_to_internal(elem->rdn_elem_id);
-                    if (0 == rdnidx) { /* Child is a Leaf RDN to be added */
-                        if (currid == id) {
-                            /* already in the file */
-                            /* do nothing and return. */
-                            rc = 0;
-                            slapi_log_error(SLAPI_LOG_BACKLDBM, ENTRYRDN_TAG,
-                                       "_entryrdn_insert_key: ID %d is already "
-                                       "in the index. NOOP.\n", currid);
-                        } else { /* different id, error return */
-                            char *dn  = NULL;
-                            int tmprc = slapi_rdn_get_dn(srdn, &dn);
-                            slapi_log_error(SLAPI_LOG_FATAL,
-                                  ENTRYRDN_TAG,
-                                  "_entryrdn_insert_key: Same DN (%s: %s) "
-                                  "is already in the %s file with different ID "
-                                  "%d.  Expected ID is %d.\n", 
-                                  tmprc?"rdn":"dn", tmprc?childnrdn:dn,
-                                  LDBM_ENTRYRDN_STR, currid, id);
-                            slapi_ch_free_string(&dn);
-                            /* returning special error code for the upgrade */
-                            rc = LDBM_ERROR_FOUND_DUPDN;
-                        }
-                        slapi_ch_free((void **)&tmpelem);
-                        goto bail;
-                    } else { /* if (0 != rdnidx) */
-                        nrdn = childnrdn;
-                        workid = currid;
-                        slapi_ch_free((void **)&parentelem);
-                        parentelem = elem;
-                        elem = NULL;
-                    }
+                    nrdn = childnrdn;
+                    workid = currid;
+                    slapi_ch_free((void **)&parentelem);
+                    parentelem = elem;
+                    elem = NULL;
                 }
             } else {
                 char *dn  = NULL;

ldap/servers/slapd/libglobs.c

@@ -5899,6 +5899,7 @@ config_set_default_naming_context(const char *attrname,
     }
 
     if (!apply) {
+        slapi_ch_free_string(&suffix);
         return LDAP_SUCCESS;
     }
 

ldap/servers/slapd/ssl.c

@@ -1497,7 +1497,6 @@ char* slapd_get_tmp_dir()
 	unsigned ilen;
 	char pch;
 #endif
-	struct stat ffinfo;
 
 	tmp[0] = '\0';
 
@@ -1535,25 +1534,32 @@ char* slapd_get_tmp_dir()
 	}
 #endif
 
-	if(stat(tmpdir, &ffinfo) == -1)
 #if defined( XP_WIN32 )
-		if(CreateDirectory(tmpdir, NULL) == 0)
-		{
-			slapi_log_error(
+	if(CreateDirectory(tmpdir, NULL) == 0)
+	{
+		slapi_log_error(
 			 SLAPI_LOG_FATAL,
 			 "slapd_get_tmp_dir",
 			 "CreateDirectory(%s, NULL) Error: %s\n",
 			 tmpdir, strerror(errno));	
-		}
+	}
 #else
-		if(mkdir(tmpdir, 00770) == -1)
-		{
+	if(mkdir(tmpdir, 00770) == -1)
+	{
+		if (errno == EEXIST) {
+			slapi_log_error(
+			 SLAPI_LOG_TRACE,
+			 "slapd_get_tmp_dir",
+			 "mkdir(%s, 00770) - already exists\n",
+			 tmpdir);
+		} else {
 			slapi_log_error(
 			 SLAPI_LOG_FATAL,
 			 "slapd_get_tmp_dir",
 			 "mkdir(%s, 00770) Error: %s\n",
-			 tmpdir, strerror(errno));	
+			 tmpdir, strerror(errno));
 		}
+	}
 #endif
 	return ( tmpdir );
 }

ldap/servers/slapd/tools/ldclt/data.c

@@ -234,18 +234,8 @@ int loadImages (
       /*
        * Read the image size
        */
-      strcpy (name, dirpath);
-      strcat (name, "/");
-      strcat (name, fileName);
-      if (stat (name, &stat_buf) < 0)
-      {
-	perror (name);
-	fprintf (stderr, "Cannot stat(%s)\n", name);
-	fflush (stderr);
-	rc = -1;
-	goto exit;
-      }
-      mctx.images[mctx.imagesNb-1].length = stat_buf.st_size;
+      snprintf (name, sizeof(name), "%s/%s", dirpath, fileName);
+      name[sizeof(name)-1] = '\0';
 
       /*
        * Open the image
@@ -260,6 +250,16 @@ int loadImages (
 	goto exit;
       }
 
+      if (fstat (fd, &stat_buf) < 0)
+      {
+	perror (name);
+	fprintf (stderr, "Cannot stat(%s)\n", name);
+	fflush (stderr);
+	rc = -1;
+	goto exit;
+      }
+      mctx.images[mctx.imagesNb-1].length = stat_buf.st_size;
+
 #ifdef _WIN32
       /*
        * Allocate buffer and read the data :-(

ldap/servers/slapd/tools/ldclt/ldapfct.c

@@ -2624,9 +2624,8 @@ doRename (
    */
   if (buildRandomRdnOrFilter (tttctx) < 0)
     return (-1);
-  strcpy (oldDn, tttctx->bufFilter);
-  strcat (oldDn, ",");
-  strcat (oldDn, tttctx->bufBaseDN);
+  snprintf (oldDn, sizeof(oldDn), "%s,%s", tttctx->bufFilter, tttctx->bufBaseDN);
+  oldDn[sizeof(oldDn)-1] = '\0';
 
   /*
    * Now, build a random new name for this entry
@@ -3585,9 +3584,8 @@ doDeleteEntry (
      */
     if (buildRandomRdnOrFilter (tttctx) < 0)
       return (-1);
-    strcpy (delDn, tttctx->bufFilter);
-    strcat (delDn, ",");
-    strcat (delDn, tttctx->bufBaseDN);
+    snprintf(delDn, sizeof(delDn), "%s,%s", tttctx->bufFilter, tttctx->bufBaseDN);
+    delDn[sizeof(delDn)-1] = '\0';
 
     ret = ldap_delete_ext_s (tttctx->ldapCtx, delDn, NULL, NULL);
     if (ret != LDAP_SUCCESS)

ldap/servers/slapd/tools/ldclt/ldclt.c

@@ -1270,6 +1270,7 @@ basicInit (void)
   int		 ret;	/* Return value */
   int		 oflags;/* open() flags */			/*JLS 05-04-01*/
   struct stat file_st ; /* file status checker for attreplacefile option */
+  int fd = -1;
   FILE *attrF;		/* file pointer for attreplacefile option */
   char	buffer[BUFFERSIZE];	/* buffer used to read attreplacefile content */
 
@@ -1539,9 +1540,18 @@ basicInit (void)
       (3) save the content into mctx.attrplFileContent 
     */
 
+    fd = open(mctx.attrplFile, O_RDONLY);
+    if (fd == -1)
+    {
+      printf("ERROR reading attr file [%s]\n",mctx.attrplFile); 
+      return (-1);
+    }else{
+      printf("file opened for reading\n");
+    }
     /* determine file size here */
-    if (stat(mctx.attrplFile, &file_st) < 0){
+    if (fstat(fd, &file_st) < 0){
       printf ("attr replace file [%s] does not exist, exit\n", mctx.attrplFile);
+      close(fd);
       return (-1);
     }else{
       mctx.attrplFileSize = file_st.st_size;
@@ -1549,9 +1559,10 @@ basicInit (void)
     }   
    
     /* open file to read */ 
-    if ((attrF = fopen(mctx.attrplFile, "r")) == NULL )
+    if ((attrF = fdopen(fd, "r")) == NULL )
     {
       printf("ERROR reading attr file [%s]\n",mctx.attrplFile); 
+      close(fd);
       return (-1);
     }else{
       printf("file opened for reading\n");

ldap/servers/slapd/tools/ldclt/scalab01.c

@@ -429,7 +429,8 @@ scalab01_addLogin (
     return -1;
   }
 
-  strcpy (new->dn, dn);
+  strncpy (new->dn, dn, sizeof(new->dn));
+  new->dn[sizeof(new->dn)-1] = '\0';
   new->cost = new->counter = duration;
   new->next = NULL;
 
@@ -537,9 +538,8 @@ scalab01_connectSuperuser (void)
   unsigned int mod2 = mctx.mod2;
 
   if (!(mode & CLTAUTH)) {
-    strcpy (bindDN, SCALAB01_SUPER_USER_RDN);
-    strcat (bindDN, ",");
-    strcat (bindDN, mctx.baseDN);
+    snprintf (bindDN, sizeof(bindDN), "%s,%s", SCALAB01_SUPER_USER_RDN, mctx.baseDN);
+    bindDN[sizeof(bindDN)-1] = '\0';
   }
   /* clear bits not applicable to this mode */
   mod2 &= ~M2_RNDBINDFILE;

ldap/servers/slapd/tools/ldclt/threadMain.c

@@ -627,7 +627,8 @@ msgIdAdd (
   tttctx->lastMsgId->next  = NULL;
   tttctx->lastMsgId->msgid = msgid;
   strcpy (tttctx->lastMsgId->str, str);
-  strcpy (tttctx->lastMsgId->dn, dn);
+  strncpy (tttctx->lastMsgId->dn, dn, sizeof(tttctx->lastMsgId->dn));
+  tttctx->lastMsgId->dn[sizeof(tttctx->lastMsgId->dn)-1] = '\0';
   tttctx->lastMsgId->attribs = attribs;
 
   return (0);

ldap/servers/slapd/util.c

@@ -50,6 +50,7 @@
 #include <pwd.h>
 #include <stdint.h>
 #endif
+#include <fcntl.h>
 #include <libgen.h>
 #include <pk11func.h>
 #include "slap.h"
@@ -876,21 +877,27 @@ strarray2str( char **a, char *buf, size_t buflen, int include_quotes )
 int
 slapd_chown_if_not_owner(const char *filename, uid_t uid, gid_t gid)
 {
+        int fd = -1;
         struct stat statbuf;
         int result = 1;
         if (!filename)
                 return result;
 
+        fd = open(filename, O_RDONLY);
+        if (fd == -1) {
+                return result;
+        }
         memset(&statbuf, '\0', sizeof(statbuf));
-        if (!(result = stat(filename, &statbuf)))
+        if (!(result = fstat(fd, &statbuf)))
         {
                 if (((uid != -1) && (uid != statbuf.st_uid)) ||
                         ((gid != -1) && (gid != statbuf.st_gid)))
                 {
-                        result = chown(filename, uid, gid);
+                        result = fchown(fd, uid, gid);
                 }
         }
 
+        close(fd);
         return result;
 }
 #endif

ldap/systools/idsktune.c

@@ -2731,7 +2731,8 @@ static int check_fs_options(char *reqdir,char mntbuf[MAXPATHLEN])
     if (mntdir_matches(reqdir,reqlen,mep->IDDS_MNTENT_DIRNAME,mntbuf) == 0) {
       found = 0;
 #if defined(IDDS_MNTENT_OPTIONS)
-      strcpy(optbuf,mep->IDDS_MNTENT_OPTIONS);
+      strncpy(optbuf,mep->IDDS_MNTENT_OPTIONS,sizeof(optbuf));
+      optbuf[sizeof(optbuf)-1] = '\0';
 #else
       strcpy(optbuf,"");
 #endif
@@ -3318,7 +3319,8 @@ int main(int argc,char *argv[])
       flag_carrier = 1;
       break;
     case 'i':
-      strcpy(install_dir,optarg);
+      strncpy(install_dir,optarg,sizeof(install_dir));
+      install_dir[sizeof(install_dir)-1] = '\0';
       break;
     default:
       usage(argv[0]);

lib/base/file.cpp

@@ -503,30 +503,6 @@ NSAPI_PUBLIC int file_notfound(void)
 #endif
 }
 
-NSAPI_PUBLIC int dir_create_all(char *dir)
-{
-    struct stat fi;
-    char *t;
-
-#ifdef XP_WIN32
-    t = dir + 3;
-#else /* XP_UNIX */
-    t = dir + 1;
-#endif
-    while(1) {
-        t = strchr(t, FILE_PATHSEP);
-        if(t) *t = '\0';
-        if(stat(dir, &fi) == -1) {
-            if(dir_create(dir) == -1)
-                return -1;
-        }
-        if(t) *t++ = FILE_PATHSEP;
-        else break;
-    }
-    return 0;
-}
-
-
 #ifdef XP_UNIX
 #if !defined(SNI) && !defined(LINUX)
 extern char *sys_errlist[];

lib/libaccess/acl.tab.cpp

@@ -1213,8 +1213,8 @@ case 13:
                 
                 if (!use_generic_rights) {
 			acl_string_lower(aclpvt[-0].string);
-			strcpy(acl_tmp_arg, "http_");
-			strcat(acl_tmp_arg, aclpvt[-0].string);
+			snprintf(acl_tmp_arg, sizeof(acl_tmp_arg), "http_%s", aclpvt[-0].string);
+			acl_tmp_arg[sizeof(acl_tmp_arg)-1] = '\0';
 			PERM_FREE(aclpvt[-0].string);
 			acl_new_arg = PERM_STRDUP(acl_tmp_arg);
 			acl_add_arg(curr_args_list, acl_new_arg);

lib/libaccess/acltext.y

@@ -350,8 +350,8 @@ arg_v2: ACL_VARIABLE_TOK
                 
                 if (!use_generic_rights) {
 			acl_string_lower($<string>1);
-			strcpy(acl_tmp_arg, "http_");
-			strcat(acl_tmp_arg, $<string>1);
+			snprintf(acl_tmp_arg, sizeof(acl_tmp_arg), "http_%s", $<string>1);
+			acl_tmp_arg[sizeof(acl_tmp_arg)-1] = '\0';
 			PERM_FREE($<string>1);
 			acl_new_arg = PERM_STRDUP(acl_tmp_arg);
 			acl_add_arg(curr_args_list, acl_new_arg);