Re: File descriptor leaks from open() in e-d-s

Hi,

On Mon, 2007-05-14 at 10:21 -0400, Jeffrey Stedfast wrote:
> On Mon, 2007-05-14 at 15:44 +0200, Jules Colding wrote:

Changed to g_mkstemp() everywhere possible.
 
I this patch better?

-- 
  jules


Index: camel/providers/nntp/ChangeLog
===================================================================
--- camel/providers/nntp/ChangeLog	(revision 7760)
+++ camel/providers/nntp/ChangeLog	(working copy)
@@ -1,3 +1,7 @@
+2007-05-14  Jules Colding  <[email protected]>
+
+	* camel-nntp-newsrc.c (camel_nntp_newsrc_read_for_server): Fix file descriptor leak
+
 2007-04-05  Ross Burton  <[email protected]>
 
 	* camel-nntp-folder.c: (camel_nntp_folder_new):
Index: camel/providers/nntp/camel-nntp-newsrc.c
===================================================================
--- camel/providers/nntp/camel-nntp-newsrc.c	(revision 7760)
+++ 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 7760)
+++ camel/providers/local/camel-spool-summary.c	(working copy)
@@ -128,7 +128,7 @@
 spool_summary_sync_full(CamelMboxSummary *cls, gboolean expunge, CamelFolderChangeInfo *changeinfo, CamelException *ex)
 {
 	int fd = -1, fdout = -1;
-	char *tmpname = NULL;
+	char tmpname[64] = { '\0' };
 	char *buffer, *p;
 	off_t spoollen, outlen;
 	int size, sizeout;
@@ -149,16 +149,9 @@
 		return -1;
 	}
 
-#ifdef HAVE_MKSTEMP
-	tmpname = alloca (64);
 	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
+	fdout = g_mkstemp (tmpname);
+
 	d(printf("Writing tmp file to %s\n", tmpname));
 	if (fdout == -1) {
 		camel_exception_setv (ex, CAMEL_EXCEPTION_SYSTEM,
@@ -245,11 +238,9 @@
 					      _("Could not sync spool folder %s: %s\n"
 						"Folder may be corrupt, copy saved in `%s'"),
 					      ((CamelLocalSummary *)cls)->folder_path,
-					      g_strerror (errno), tmpnam);
+					      g_strerror (errno), tmpname);
 			/* so we dont delete it */
-			close(fdout);
-			tmpname = NULL;
-			fdout = -1;
+			tmpname[0] = '\0';
 			g_free(buffer);
 			goto error;
 		}
@@ -264,10 +255,8 @@
 				      _("Could not sync spool folder %s: %s\n"
 					"Folder may be corrupt, copy saved in `%s'"),
 				      ((CamelLocalSummary *)cls)->folder_path,
-				      g_strerror (errno), tmpnam);
-		close(fdout);
-		tmpname = NULL;
-		fdout = -1;
+				      g_strerror (errno), tmpname);
+		tmpname[0] = '\0';
 		goto error;
 	}
 
@@ -277,17 +266,17 @@
 				      _("Could not sync spool folder %s: %s\n"
 					"Folder may be corrupt, copy saved in `%s'"),
 				      ((CamelLocalSummary *)cls)->folder_path,
-				      g_strerror (errno), tmpnam);
-		close(fdout);
-		tmpname = NULL;
-		fdout = -1;
+				      g_strerror (errno), tmpname);
+		tmpname[0] = '\0';
 		fd = -1;
 		goto error;
 	}
 
 	close(fdout);
-	unlink(tmpname);
 
+	if (tmpname[0] != '\0')
+		unlink(tmpname);
+
 	camel_operation_end(NULL);
 		
 	return 0;
@@ -298,7 +287,7 @@
 	if (fdout != -1)
 		close(fdout);
 	
-	if (tmpname)
+	if (tmpname[0] != '\0')
 		unlink(tmpname);
 
 	camel_operation_end(NULL);
Index: camel/providers/local/camel-mh-summary.c
===================================================================
--- camel/providers/local/camel-mh-summary.c	(revision 7760)
+++ 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 7760)
+++ camel/providers/local/ChangeLog	(working copy)
@@ -1,3 +1,16 @@
+2007-05-16  Jules Colding  <[email protected]>
+
+	* camel-spool-summary.c (spool_summary_sync_full): Use g_mkstemp()
+
+2007-05-14  Jules Colding  <[email protected]>
+
+	* 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  <[email protected]>
 
 	* camel-local-store.c: (get_folder):
Index: camel/ChangeLog
===================================================================
--- camel/ChangeLog	(revision 7760)
+++ camel/ChangeLog	(working copy)
@@ -1,3 +1,27 @@
+2007-05-16  Jules Colding  <[email protected]>
+
+	* camel-lock.c (camel_lock_dot): Use g_mkstemp()
+
+	* 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
+ 
 2007-05-15  Srinivasa Ragavan  <[email protected]>
 
 
@@ -6,12 +30,6 @@
 	* camel-disco-store.h:
 
 2007-05-12  Jules Colding  <[email protected]>
- 
-	* camel-lock-client.c (camel_lock_helper_init): Fix file descriptor leak
- 
-	* camel-process.c (camel_process_fork): Fix file descriptor leak
- 
-2007-05-12  Jules Colding  <[email protected]>
 
 	* camel-store.h (CamelFolderInfo): Change type of unread and total 
 	members to gint32 instead of guint32. The previous unsigned members 
Index: camel/camel-lock.c
===================================================================
--- camel/camel-lock.c	(revision 7760)
+++ camel/camel-lock.c	(working copy)
@@ -89,17 +89,6 @@
 	sprintf(lock, "%s.lock", path);
 	locktmp = alloca(strlen(path) + strlen("XXXXXX") + 1);
 
-#ifndef HAVE_MKSTEMP
-	sprintf(locktmp, "%sXXXXXX", path);
-	if (mktemp(locktmp) == NULL) {
-		/* well, this is really only a programatic error */
-		camel_exception_setv (ex, CAMEL_EXCEPTION_SYSTEM,
-				      _("Could not create lock file for %s: %s"),
-				      path, g_strerror (errno));
-		return -1;
-	}
-#endif
-
 	while (retry < CAMEL_LOCK_DOT_RETRY) {
 
 		d(printf("trying to lock '%s', attempt %d\n", lock, retry));
@@ -107,12 +96,8 @@
 		if (retry > 0)
 			sleep(CAMEL_LOCK_DOT_DELAY);
 
-#ifdef HAVE_MKSTEMP
 		sprintf(locktmp, "%sXXXXXX", path);
-		fdtmp = mkstemp(locktmp);
-#else
-		fdtmp = open(locktmp, O_RDWR|O_CREAT|O_EXCL, 0600);
-#endif
+		fdtmp = g_mkstemp(locktmp);
 		if (fdtmp == -1) {
 			camel_exception_setv (ex, CAMEL_EXCEPTION_SYSTEM,
 					      _("Could not create lock file for %s: %s"),
@@ -372,7 +357,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 7760)
+++ 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 7760)
+++ camel/tests/ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2007-05-14  Jules Colding  <[email protected]>
+
+	* mime-filter/test-charset.c (main): Check return value
+
+	* message/test2.c (convert): Check return value
+
 2006-06-22  Harish Krishnaswamy  <[email protected]>
 
 	* Makefile.am: Remove CVS/*, .cvsignore files from
Index: camel/tests/mime-filter/test-charset.c
===================================================================
--- camel/tests/mime-filter/test-charset.c	(revision 7760)
+++ 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 7760)
+++ 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 7760)
+++ 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 7760)
+++ 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 7760)
+++ 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 7760)
+++ 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 7760)
+++ 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 7760)
+++ ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2007-05-14  Jules Colding  <[email protected]>
+
+	* 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 7760)
+++ 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 7760)
+++ 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 7760)
+++ 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 7760)
+++ 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 7760)
+++ addressbook/ChangeLog	(working copy)
@@ -1,3 +1,10 @@
+2007-05-16  Jules Colding  <[email protected]>
+
+	* 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-15  Ross Burton  <[email protected]>
 
 	* tests/ebook/test-ebook-async.c:
Index: addressbook/backends/vcf/e-book-backend-vcf.c
===================================================================
--- addressbook/backends/vcf/e-book-backend-vcf.c	(revision 7760)
+++ 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 7760)
+++ calendar/ChangeLog	(working copy)
@@ -1,3 +1,14 @@
+2007-05-16  Jules Colding  <[email protected]>
+
+	* 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-15  Ross Burton  <[email protected]>
 
 	* tests/ecal/test-ecal.c:
Index: calendar/backends/file/e-cal-backend-file.c
===================================================================
--- calendar/backends/file/e-cal-backend-file.c	(revision 7760)
+++ 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 7760)
+++ 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 7760)
+++ 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);