Re: [evolution-patches] File descriptor leaks in e-d-s

[Jeffrey Stedfast <fejj novell com>]

On Fri, 2007-05-11 at 13:04 +0200, Jules Colding wrote:
> Hi,
> 
> I'm plagued by Evolution leaking file descriptors. I'm currently running
> 2.8.2.1 on Gentoo, but having the updated e-d-s svn tree in front of me
> I digged into that and reviewed it for leaks involving pipe(2). 
> 
> I found a small number of such leaks in the latest svn tree. Most of
> them was due to a missing close(2) when returning from an error
> condition. The patch below (#437665) should fix them all. Should I
> commit to svn?
> 
> Comments and feedback much appreciated.
> 
> Best regards,
>   jules
> 
> 
> Index: camel/providers/sendmail/camel-sendmail-transport.c
> ===================================================================
> --- camel/providers/sendmail/camel-sendmail-transport.c	(revision 7735)
> +++ camel/providers/sendmail/camel-sendmail-transport.c	(working copy)
> @@ -163,6 +163,8 @@
>  	pid = fork ();
>  	switch (pid) {
>  	case -1:
> +		close (fd[0]);
> +		close (fd[1]);
>  		camel_exception_setv (ex, CAMEL_EXCEPTION_SYSTEM,
>  				      _("Could not fork sendmail: "
>  					"%s: mail not sent"),

you need to close the fds after setting the exception or errno will get
cleared/won't refer to the fork() error.

> Index: camel/providers/sendmail/ChangeLog
> ===================================================================
> --- camel/providers/sendmail/ChangeLog	(revision 7735)
> +++ camel/providers/sendmail/ChangeLog	(working copy)
> @@ -1,3 +1,7 @@
> +2007-05-11  Jules Colding  <colding omesc com>
> +
> +	* camel-sendmail-transport.c (sendmail_send_to): Fix file descriptor leak
> +
>  2005-09-16  Tor Lillqvist  <tml novell com>
>  
>  	* camel-sendmail-transport.c: Use g_ascii_strcasecmp() instead of
> @@ -14,4 +18,4 @@
>  	* camel-sendmail-provider.c (camel_provider_module_init):
>  	set translation_domain in CamelProvider struct.
>  
> -** refer to main changelog for earlier changes
> \ No newline at end of file
> +** refer to main changelog for earlier changes
> Index: camel/ChangeLog
> ===================================================================
> --- camel/ChangeLog	(revision 7735)
> +++ camel/ChangeLog	(working copy)
> @@ -1,3 +1,9 @@
> +2007-05-11  Jules Colding  <colding omesc com>
> +
> +	* camel-lock-client.c (camel_lock_helper_init): Fix file descriptor leak
> +
> +	* camel-process.c (camel_process_fork): Fix file descriptor leak
> +
>  2007-05-07  Matthew Barnes  <mbarnes redhat com>
>  
>  	* camel-stream-vfs.c:
> Index: camel/camel-process.c
> ===================================================================
> --- camel/camel-process.c	(revision 7735)
> +++ camel/camel-process.c	(working copy)
> @@ -86,6 +86,9 @@
>  		execv (path, argv);
>  		_exit (255);
>  	} else if (pid == -1) {
> +		for (i = 0; i < 6; i++) 
> +			close (fd[i]);
> +
>  		camel_exception_setv (ex, CAMEL_EXCEPTION_SYSTEM,
>  				      _("Failed to create child process '%s': %s"),
>  				      argv[0], strerror (errno));


you either need to save errno or set the exception first, and then close
all the pipes. Otherwise you'd get error dialogs like "Failed to do XYZ:
Success"


> Index: camel/camel-lock-client.c
> ===================================================================
> --- camel/camel-lock-client.c	(revision 7735)
> +++ camel/camel-lock-client.c	(working copy)
> @@ -94,8 +94,21 @@
>  {
>  	int i;
>  
> +	lock_stdin_pipe[0] = -1;
> +	lock_stdin_pipe[1] = -1;
> +	lock_stdout_pipe[0] = -1;
> +	lock_stdout_pipe[1] = -1;
>  	if (pipe(lock_stdin_pipe) == -1
>  	    || pipe(lock_stdout_pipe) == -1) {
> +		if (-1 != lock_stdin_pipe[0])
> +			close(lock_stdin_pipe[0]);
> +		if (-1 != lock_stdin_pipe[1])
> +			close(lock_stdin_pipe[1]);
> +		if (-1 != lock_stdout_pipe[0])
> +			close(lock_stdout_pipe[0]);
> +		if (-1 != lock_stdout_pipe[1])
> +			close(lock_stdout_pipe[1]);

same as above, but also stylistically, camel (and all of evolution
afaik) checks:

if (lock_stdin_pipe[0] != -1)

as it reads easier

> +
>  		camel_exception_setv (ex, CAMEL_EXCEPTION_SYSTEM,
>  				      _("Cannot build locking helper pipe: %s"),
>  				      g_strerror (errno));
> 
> 
> _______________________________________________
> Evolution-patches mailing list
> Evolution-patches gnome org
> http://mail.gnome.org/mailman/listinfo/evolution-patches