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

On Mon, 2007-05-14 at 15:44 +0200, Jules Colding wrote:
> 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  <[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 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);

should never cast the return value from malloc & friends, it only serves
to hide compiler warnings that are real bugs (e.g. not including proper
headers).

> +#endif
> +	if (!tmpname) {
> +		camel_exception_setv (ex, CAMEL_EXCEPTION_SYSTEM,
> +				      _("Cannot open temporary mailbox: %s"),
> +				      g_strerror (errno));
> +		goto error;
> +	}
> +#ifdef HAVE_MKSTEMP

I'm thinking that instead of calling malloc (since the rest of evolution
does not), that the above code should not be calling malloc() either.

I would suggest sticking with alloca() (or g_alloca()), or perhaps
changing the declaration of tmpname to something like:

#if HAVE_MKSTEMP
	char tmpname[64];
#else
	char tmpname[L_tmpnam];
#endif

>  	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

hmmm, I wonder if glib has a g_mkstemp() or something? if not, perhaps
we should consider writing our own mkstemp() wrapper in
camel-file-utils.c or something.

if there is a g_mkstemp(), this code could be simplified a bit and
probably not have to alloca() at all.

> @@ -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);
> +	}

ah, hmm... this is why alloca() was used. doesn't mean you can't use a
similar trick tho, like setting tmpname[0] = 0; and checking that.

>  	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  <[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 7749)
> +++ camel/ChangeLog	(working copy)
> @@ -1,5 +1,21 @@
> -2007-05-12  Jules Colding  <[email protected]>
> - 
> +2007-05-14  Jules Colding  <[email protected]>
> +
> +	* 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  <[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 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  <[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 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;
>  }

I wonder if it isn't best to just always return 0 here. there's nothing
that can be done by the caller if we return -1.

>  
>  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  <[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-04  Milan Crha  <[email protected]>
>  
>  	** 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  <[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-13  Rob Bradford  <[email protected]>
>  
>  	* 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);
> 
> 
> _______________________________________________
> Evolution-patches mailing list
> [email protected]
> http://mail.gnome.org/mailman/listinfo/evolution-patches