[evolution-patches] File descriptor leaks from open() in e-d-s

[Jules Colding <colding omesc com>]

Hi,

The following patch represents a full review of the current svn e-d-s
source tree for file descriptor leaks introduced by open() and
friends.  

All of e-d-s has been reviewed except libdb. Most test code has been
left unchanged even in the presence of obvious leaks. The reason is that
the test code more often than not are small standalone applications
where the return from main() will close all remaining open file
descriptors.

I've done a little cleanup in the most obvious cases but has otherwise
attempted to leave my own opinion of coding style and looks out of the
review.

Please review the patch for blunders and errors.

May I commit?

Thanks,
  jules



Index: camel/providers/nntp/ChangeLog
===================================================================
--- camel/providers/nntp/ChangeLog	(revision 7749)
+++ camel/providers/nntp/ChangeLog	(working copy)
@@ -1,3 +1,7 @@
+2007-05-14  Jules Colding  <colding omesc com>
+
+	* camel-nntp-newsrc.c (camel_nntp_newsrc_read_for_server): Fix file descriptor leak
+
 2007-04-05  Ross Burton  <ross openedhand com>
 
 	* camel-nntp-folder.c: (camel_nntp_folder_new):
Index: camel/providers/nntp/camel-nntp-newsrc.c
===================================================================
--- camel/providers/nntp/camel-nntp-newsrc.c	(revision 7749)
+++ camel/providers/nntp/camel-nntp-newsrc.c	(working copy)
@@ -622,6 +622,7 @@
 
 	if (fstat (fd, &sb) == -1) {
 		g_warning ("failed fstat on ~/.newsrc-%s: %s\n", server, strerror(errno));
+		close (fd);
 		return newsrc;
 	}
 	newsrc_len = sb.st_size;
Index: camel/providers/local/camel-spool-summary.c
===================================================================
--- camel/providers/local/camel-spool-summary.c	(revision 7749)
+++ camel/providers/local/camel-spool-summary.c	(working copy)
@@ -150,12 +150,21 @@
 	}
 
 #ifdef HAVE_MKSTEMP
-	tmpname = alloca (64);
+	tmpname = (char*)malloc (64);
+#else
+	tmpname = (char*)malloc (L_tmpnam);
+#endif
+	if (!tmpname) {
+		camel_exception_setv (ex, CAMEL_EXCEPTION_SYSTEM,
+				      _("Cannot open temporary mailbox: %s"),
+				      g_strerror (errno));
+		goto error;
+	}
+#ifdef HAVE_MKSTEMP
 	sprintf (tmpname, "/tmp/spool.camel.XXXXXX");
 	fdout = mkstemp (tmpname);
 #else
 #warning "Your system has no mkstemp(3), spool updating may be insecure"
-	tmpname = alloca (L_tmpnam);
 	tmpnam (tmpname);
 	fdout = open (tmpname, O_RDWR|O_CREAT|O_EXCL, 0600);
 #endif
@@ -247,9 +256,8 @@
 					      ((CamelLocalSummary *)cls)->folder_path,
 					      g_strerror (errno), tmpnam);
 			/* so we dont delete it */
-			close(fdout);
+			free(tmpname);
 			tmpname = NULL;
-			fdout = -1;
 			g_free(buffer);
 			goto error;
 		}
@@ -265,9 +273,8 @@
 					"Folder may be corrupt, copy saved in `%s'"),
 				      ((CamelLocalSummary *)cls)->folder_path,
 				      g_strerror (errno), tmpnam);
-		close(fdout);
+		free(tmpname);
 		tmpname = NULL;
-		fdout = -1;
 		goto error;
 	}
 
@@ -278,16 +285,17 @@
 					"Folder may be corrupt, copy saved in `%s'"),
 				      ((CamelLocalSummary *)cls)->folder_path,
 				      g_strerror (errno), tmpnam);
-		close(fdout);
+		free(tmpname);
 		tmpname = NULL;
-		fdout = -1;
 		fd = -1;
 		goto error;
 	}
 
 	close(fdout);
-	unlink(tmpname);
-
+	if (tmpname) {
+		unlink(tmpname);
+		free(tmpname);
+	}
 	camel_operation_end(NULL);
 		
 	return 0;
@@ -298,8 +306,10 @@
 	if (fdout != -1)
 		close(fdout);
 	
-	if (tmpname)
+	if (tmpname) {
 		unlink(tmpname);
+		free(tmpname);
+	}
 
 	camel_operation_end(NULL);
 
Index: camel/providers/local/camel-mh-summary.c
===================================================================
--- camel/providers/local/camel-mh-summary.c	(revision 7749)
+++ camel/providers/local/camel-mh-summary.c	(working copy)
@@ -147,7 +147,8 @@
 	} else {
 		/* else scan for one - and create it too, to make sure */
 		do {
-			close(fd);
+			if (fd != -1)
+				close(fd);
 			uid = camel_folder_summary_next_uid(s);
 			name = g_strdup_printf("%s/%u", cls->folder_path, uid);
 			/* O_EXCL isn't guaranteed, sigh.  Oh well, bad luck, mh has problems anyway */
@@ -155,7 +156,8 @@
 			g_free(name);
 		} while (fd == -1 && errno == EEXIST);
 
-		close(fd);
+		if (fd != -1)
+			close(fd);
 
 		uidstr = g_strdup_printf("%u", uid);
 	}
Index: camel/providers/local/ChangeLog
===================================================================
--- camel/providers/local/ChangeLog	(revision 7749)
+++ camel/providers/local/ChangeLog	(working copy)
@@ -1,3 +1,12 @@
+2007-05-14  Jules Colding  <colding omesc com>
+
+	* camel-mh-summary.c (mh_summary_next_uid_string): Do not close() if (fd == -1).
+
+	* camel-spool-summary.c (spool_summary_sync_full): alloca() changed into malloc(). 
+	alloca() has undefined behaviour for stack overflow so it shouldn't really be used 
+	unless there is a *very* good reason. Cleanups related to this fix as well. Do not 
+	close() if (fd == -1).
+
 2007-04-05  Ross Burton  <ross openedhand com>
 
 	* camel-local-store.c: (get_folder):
Index: camel/ChangeLog
===================================================================
--- camel/ChangeLog	(revision 7749)
+++ camel/ChangeLog	(working copy)
@@ -1,5 +1,21 @@
-2007-05-12  Jules Colding  <colding omesc com>
- 
+2007-05-14  Jules Colding  <colding omesc com>
+
+	* camel-folder-summary.c (camel_folder_summary_save): Fix file descriptor leak. 
+	fclose() a fdopen() stream instead of close()'ing the underlying file descriptor.
+
+	* camel-uid-cache.c (camel_uid_cache_save): Cleanups, such as 
+	setting cache->fd = -1 when fd has been closed.
+
+	* camel-block-file.c (key_file_use): Do not close() if (fd == -1)
+
+	* camel-index-control.c (do_perf): Fix file descriptor leak
+
+	* camel-charset-map.c (main): Check for return value
+
+	* camel-text-index.c (dump_raw): Fix file descriptor leak
+
+	* camel-lock.c (main): Fix file descriptor leak.
+
 	* camel-lock-client.c (camel_lock_helper_init): Fix file descriptor leak
  
 	* camel-process.c (camel_process_fork): Fix file descriptor leak
Index: camel/camel-lock.c
===================================================================
--- camel/camel-lock.c	(revision 7749)
+++ camel/camel-lock.c	(working copy)
@@ -372,7 +372,16 @@
 #endif
 
 	fd1 = open("mylock", O_RDWR);
+	if (fd1 == -1) {
+		printf("Could not open lock file (mylock): %s", g_strerror (errno));
+		return 1;
+	}
 	fd2 = open("mylock", O_RDWR);
+	if (fd2 == -1) {		
+		printf("Could not open lock file (mylock): %s", g_strerror (errno));
+		close (fd1);
+		return 1;
+	}
 
 	if (camel_lock_fcntl(fd1, CAMEL_LOCK_WRITE, ex) == 0) {
 		printf("got fcntl write lock once\n");
Index: camel/tests/message/test2.c
===================================================================
--- camel/tests/message/test2.c	(revision 7749)
+++ camel/tests/message/test2.c	(working copy)
@@ -52,6 +52,9 @@
 		char *nout, *noutp;
 		iconv_t ic = iconv_open(from, to);
 
+		if (ic == (iconv_t)-1)
+			goto fail;
+
 		inp = out;
 		inlen = strlen(out);
 		outlen = inlen*5 + 16;
@@ -68,6 +71,7 @@
 		printf("Camel thinks the best encoding of '%s' is %s, although we converted from %s\n",
 		       in, camel_charset_best(out, strlen(out)), from);
 	}
+fail:
 #endif
 
 	return out;
Index: camel/tests/ChangeLog
===================================================================
--- camel/tests/ChangeLog	(revision 7749)
+++ camel/tests/ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2007-05-14  Jules Colding  <colding omesc com>
+
+	* mime-filter/test-charset.c (main): Check return value
+
+	* message/test2.c (convert): Check return value
+
 2006-06-22  Harish Krishnaswamy  <kharish novell com>
 
 	* Makefile.am: Remove CVS/*, .cvsignore files from
Index: camel/tests/mime-filter/test-charset.c
===================================================================
--- camel/tests/mime-filter/test-charset.c	(revision 7749)
+++ camel/tests/mime-filter/test-charset.c	(working copy)
@@ -36,6 +36,8 @@
 	camel_test_init(argc, argv);
 	
 	dir = opendir (SOURCEDIR);
+	if (!dir)
+		return 1;
 	
 	while ((dent = readdir (dir))) {
 		char *infile, *outfile, *charset, *work;
Index: camel/camel-text-index.c
===================================================================
--- camel/camel-text-index.c	(revision 7749)
+++ camel/camel-text-index.c	(working copy)
@@ -1172,6 +1172,7 @@
 		} while (len);
 		printf("\n");
 	}
+	close (fd);
 }
 #endif
 
@@ -1937,6 +1938,7 @@
 
 		index++;
 	}
+	fclose (fp);
 
 	printf("Freeing partition index\n");
 	camel_partition_table_free(cpi);
Index: camel/camel-charset-map.c
===================================================================
--- camel/camel-charset-map.c	(revision 7749)
+++ camel/camel-charset-map.c	(working copy)
@@ -106,6 +106,8 @@
 
 	for (j = 0; tables[j].name; j++) {
 		cd = iconv_open (UCS, tables[j].name);
+		if (cd == (iconv_t)-1)
+			exit (1);
 		inptr = in;
 		outptr = (char *)(out);
 		inlen = sizeof (in);
Index: camel/camel-index-control.c
===================================================================
--- camel/camel-index-control.c	(revision 7749)
+++ camel/camel-index-control.c	(working copy)
@@ -172,6 +172,7 @@
 	idx = (CamelIndex *)camel_text_index_new("/tmp/index", O_TRUNC|O_CREAT|O_RDWR);
 	if (idx == NULL) {
 		perror("open index");
+		closedir(dir);
 		return 1;
 	}
 
Index: camel/camel-block-file.c
===================================================================
--- camel/camel-block-file.c	(revision 7749)
+++ camel/camel-block-file.c	(working copy)
@@ -916,7 +916,8 @@
 	if ((fd = g_open(bs->path, bs->flags|O_BINARY, 0600)) == -1
 	    || (bs->fp = fdopen(fd, flag)) == NULL) {
 		err = errno;
-		close(fd);
+		if (fd != -1)
+			close(fd);
 		CAMEL_KEY_FILE_UNLOCK(bs, lock);
 		errno = err;
 		return -1;
Index: camel/camel-uid-cache.c
===================================================================
--- camel/camel-uid-cache.c	(revision 7749)
+++ camel/camel-uid-cache.c	(working copy)
@@ -169,15 +169,13 @@
 	cache->expired = 0;
 	g_hash_table_foreach (cache->uids, maybe_write_uid, cache);
 	
-	if (cache->fd == -1)
-		goto exception;
-	
 	if (fsync (fd) == -1)
 		goto exception;
 	
 	close (fd);
 	fd = -1;
-	
+	cache->fd = -1;
+
 	if (g_rename (filename, cache->filename) == -1)
 		goto exception;
 	
@@ -211,14 +209,16 @@
 			if (ftruncate (fd, (off_t) cache->size) != -1) {
 				cache->size = 0;
 				cache->expired = 0;
-				goto overwrite;
+				goto overwrite; /* FIXME: no such label */
 			}
-		}
-		
+		}		
+	}
+#endif
+	if (fd != -1) {
 		close (fd);
+		cache->fd = -1;
 	}
-#endif
-	
+
 	g_unlink (filename);
 	g_free (filename);
 	
Index: camel/camel-folder-summary.c
===================================================================
--- camel/camel-folder-summary.c	(revision 7749)
+++ camel/camel-folder-summary.c	(working copy)
@@ -687,14 +687,16 @@
 	path_meta = alloca(strlen(s->meta_summary->path)+4);
 	sprintf(path_meta, "%s~", s->meta_summary->path);
 	fd_meta = g_open(path_meta, O_RDWR|O_CREAT|O_TRUNC|O_BINARY, 0600);
-	if (fd_meta == -1)
+	if (fd_meta == -1) {
+		fclose(out);
 		return -1;
+	}
 	out_meta = fdopen(fd_meta, "wb");
 	if (out_meta == NULL) {
 		i = errno;
 		g_unlink(path);
 		g_unlink(path_meta);
-		close(fd);
+		fclose(out);
 		close(fd_meta);
 		errno = i;
 		return -1;
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 7749)
+++ ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2007-05-14  Jules Colding  <colding omesc com>
+
+	* libedataserver/e-db3-utils.c (cp_file): Cleanup. Fix file descriptor leak.
+
+	* libedataserver/md5-utils.c (md5_get_digest_from_file): Fix file descriptor leak
+
 2007-05-14  Srinivasa Ragavan
 	
 	* NEWS, configure.in: Evolution Data Server 1.11.2 release.
Index: libedataserver/md5-utils.c
===================================================================
--- libedataserver/md5-utils.c	(revision 7749)
+++ libedataserver/md5-utils.c	(working copy)
@@ -340,7 +340,7 @@
 	md5_init (&ctx);
 	fp = g_fopen(filename, "rb");
 	if (!fp) {
-	return;
+		return;
 	}
 	
 	while ((nb_bytes_read = fread (tmp_buf, 1, sizeof (tmp_buf), fp)) > 0)
@@ -350,7 +350,7 @@
 		fclose(fp);
 		return;
 	}
-
+	fclose(fp);
 	
 	md5_final (&ctx, digest);
 }
Index: libedataserver/e-xml-utils.c
===================================================================
--- libedataserver/e-xml-utils.c	(revision 7749)
+++ libedataserver/e-xml-utils.c	(working copy)
@@ -107,7 +107,7 @@
 		errno = errnosave;
 		return -1;
 	}
-	
+
 	while ((ret = close (fd)) == -1 && errno == EINTR)
 		;
 	
Index: libedataserver/e-db3-utils.c
===================================================================
--- libedataserver/e-db3-utils.c	(revision 7749)
+++ libedataserver/e-db3-utils.c	(working copy)
@@ -83,11 +83,10 @@
 			place += count;
 		}
 	}
-	if (close (i))
-		return -1;
-	if (close (o))
-		return -1;
-	return 0;
+	i = close (i);
+	if (close (o) == -1)
+		i = -1;
+	return (i == -1) ? -1 : 0;
 }
 
 static int
@@ -98,10 +97,7 @@
 	if (o == -1)
 		return -1;
 
-	if (close (o) == -1)
-		return -1;
-
-	return 0;
+	return close (o);
 }
 
 static int
Index: addressbook/tests/vcard/dump-vcard.c
===================================================================
--- addressbook/tests/vcard/dump-vcard.c	(revision 7749)
+++ addressbook/tests/vcard/dump-vcard.c	(working copy)
@@ -23,6 +23,7 @@
 		if (fgets (buf, sizeof(buf), fp))
 			str = g_string_append (str, buf);
 	}
+	fclose (fp);
 
 	vcard = e_vcard_new_from_string (str->str);
 
Index: addressbook/ChangeLog
===================================================================
--- addressbook/ChangeLog	(revision 7749)
+++ addressbook/ChangeLog	(working copy)
@@ -1,3 +1,10 @@
+2007-05-14  Jules Colding  <colding omesc com>
+
+	* backends/vcf/e-book-backend-vcf.c (load_file): Fix file descriptor leak.
+	(save_file): Cleanup. Fix file descriptor leak.
+
+	* tests/vcard/dump-vcard.c (main): Fix file descriptor leak
+
 2007-05-04  Milan Crha  <mcrha redhat com>
 
 	** Fix for bug #274035 from Jonty Wareing
Index: addressbook/backends/vcf/e-book-backend-vcf.c
===================================================================
--- addressbook/backends/vcf/e-book-backend-vcf.c	(revision 7749)
+++ addressbook/backends/vcf/e-book-backend-vcf.c	(working copy)
@@ -110,6 +110,7 @@
 
 	fp = fdopen (fd, "rb");
 	if (!fp) {
+		close (fd); /* callers depend on fd being closed by this function */
 		g_warning ("failed to open `%s' for reading", vcf->priv->filename);
 		return;
 	}
@@ -140,6 +141,7 @@
 static gboolean
 save_file (EBookBackendVCF *vcf)
 {
+	gboolean retv = FALSE;
 	GList *l;
 	char *new_path;
 	int fd, rv;
@@ -151,6 +153,10 @@
 	new_path = g_strdup_printf ("%s.new", vcf->priv->filename);
 
 	fd = g_open (new_path, O_CREAT | O_TRUNC | O_WRONLY | O_BINARY, 0666);
+	if (fd == -1) {
+		g_warning ("write failed.  could not open output file\n");
+		goto out;
+	}
 
 	for (l = vcf->priv->contact_list; l; l = l->next) {
 		char *vcard_str = l->data;
@@ -161,34 +167,34 @@
 		if (rv < len) {
 			/* XXX */
 			g_warning ("write failed.  we need to handle short writes\n");
-			close (fd);
 			g_unlink (new_path);
-			return FALSE;
+			goto out;
 		}
 
 		rv = write (fd, "\r\n\r\n", 4);
 		if (rv < 4) {
 			/* XXX */
 			g_warning ("write failed.  we need to handle short writes\n");
-			close (fd);
 			g_unlink (new_path);
-			return FALSE;
+			goto out;
 		}
 	}
 
 	if (0 > g_rename (new_path, vcf->priv->filename)) {
 		g_warning ("Failed to rename %s: %s\n", vcf->priv->filename, strerror(errno));
 		g_unlink (new_path);
-		return FALSE;
+		goto out;
 	}
+	retv = TRUE;
 
+out:
+	if (fd != -1)
+		close (fd);
 	g_free (new_path);
-
-	vcf->priv->dirty = FALSE;
-
+	vcf->priv->dirty = !retv;
 	g_mutex_unlock (vcf->priv->mutex);
 
-	return TRUE;
+	return retv;
 }
 
 static gboolean
Index: calendar/ChangeLog
===================================================================
--- calendar/ChangeLog	(revision 7749)
+++ calendar/ChangeLog	(working copy)
@@ -1,3 +1,14 @@
+2007-05-14  Jules Colding  <colding omesc com>
+
+	* backends/groupwise/e-cal-backend-groupwise.c (fetch_attachments): 
+	Do not close() if (fd == -1)
+
+	* backends/groupwise/e-cal-backend-groupwise-utils.c (set_attachments_to_cal_component): 
+	Do not close() if (fd == -1)
+
+	* backends/file/e-cal-backend-file.c (fetch_attachments): 
+	Do not close() if (fd == -1)
+
 2007-05-13  Rob Bradford  <rob openedhand com>
 
 	* libecal/e-cal.c: (e_cal_create_object):
Index: calendar/backends/file/e-cal-backend-file.c
===================================================================
--- calendar/backends/file/e-cal-backend-file.c	(revision 7749)
+++ calendar/backends/file/e-cal-backend-file.c	(working copy)
@@ -2352,7 +2352,8 @@
 		}
 
 		g_mapped_file_free (mapped_file);
-		close (fd);
+		if (fd != -1)
+			close (fd);
 		dest_url = g_filename_to_uri (dest_file, NULL, NULL);
 		g_free (dest_file);
 		new_attach_list = g_slist_append (new_attach_list, dest_url);
Index: calendar/backends/groupwise/e-cal-backend-groupwise-utils.c
===================================================================
--- calendar/backends/groupwise/e-cal-backend-groupwise-utils.c	(revision 7749)
+++ calendar/backends/groupwise/e-cal-backend-groupwise-utils.c	(working copy)
@@ -833,7 +833,8 @@
 				g_warning ("DEBUG: attachment write failed.\n");
 			}
 			g_free (attach_data);
-			close (fd);
+			if (fd != -1)
+				close (fd);
 		}
 		g_free (filename);
 
Index: calendar/backends/groupwise/e-cal-backend-groupwise.c
===================================================================
--- calendar/backends/groupwise/e-cal-backend-groupwise.c	(revision 7749)
+++ calendar/backends/groupwise/e-cal-backend-groupwise.c	(working copy)
@@ -2279,7 +2279,8 @@
 		}
 
 		g_mapped_file_free (mapped_file);
-		close (fd);
+		if (fd != -1)
+			close (fd);
 		dest_url = g_filename_to_uri (dest_file, NULL, NULL);
 		g_free (dest_file);
 		new_attach_list = g_slist_append (new_attach_list, dest_url);