You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by da...@apache.org on 2011/07/22 20:06:13 UTC
svn commit: r1149675 - in /subversion/trunk/subversion: libsvn_repos/hooks.c
libsvn_subr/io.c
Author: danielsh
Date: Fri Jul 22 18:06:12 2011
New Revision: 1149675
URL: http://svn.apache.org/viewvc?rev=1149675&view=rev
Log:
Add error checking. This manifested as a segfault and a file descriptor leak
in svnsync of an ra_serf source to an ra_local target.
* subversion/libsvn_subr/io.c
(svn_io_start_cmd2): Check the return value of apr_procattr_io_set().
* subversion/libsvn_repos/hooks.c
(run_hook_cmd): Do not try to use CMD_PROC when it may not have been initialized.
Modified:
subversion/trunk/subversion/libsvn_repos/hooks.c
subversion/trunk/subversion/libsvn_subr/io.c
Modified: subversion/trunk/subversion/libsvn_repos/hooks.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/hooks.c?rev=1149675&r1=1149674&r2=1149675&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/hooks.c (original)
+++ subversion/trunk/subversion/libsvn_repos/hooks.c Fri Jul 22 18:06:12 2011
@@ -202,7 +202,8 @@ run_hook_cmd(svn_string_t **result,
if (err)
{
- err = svn_error_createf
+ /* CMD_PROC is not safe to use. Bail. */
+ return svn_error_createf
(SVN_ERR_REPOS_HOOK_FAILURE, err, _("Failed to start '%s' hook"), cmd);
}
else
Modified: subversion/trunk/subversion/libsvn_subr/io.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/io.c?rev=1149675&r1=1149674&r2=1149675&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/io.c (original)
+++ subversion/trunk/subversion/libsvn_subr/io.c Fri Jul 22 18:06:12 2011
@@ -2489,10 +2489,17 @@ svn_io_start_cmd2(apr_proc_t *cmd_proc,
/* Forward request for pipes to APR. */
if (infile_pipe || outfile_pipe || errfile_pipe)
- apr_procattr_io_set(cmdproc_attr,
- infile_pipe ? APR_FULL_BLOCK : APR_NO_PIPE,
- outfile_pipe ? APR_FULL_BLOCK : APR_NO_PIPE,
- errfile_pipe ? APR_FULL_BLOCK : APR_NO_PIPE);
+ {
+ apr_err = apr_procattr_io_set(cmdproc_attr,
+ infile_pipe ? APR_FULL_BLOCK : APR_NO_PIPE,
+ outfile_pipe ? APR_FULL_BLOCK : APR_NO_PIPE,
+ errfile_pipe ? APR_FULL_BLOCK : APR_NO_PIPE);
+
+ if (apr_err)
+ return svn_error_wrap_apr(apr_err,
+ _("Can't set process '%s' stdio pipes"),
+ cmd);
+ }
/* Have the child print any problems executing its program to errfile. */
apr_err = apr_pool_userdata_set(errfile, ERRFILE_KEY, NULL, pool);
Re: svn commit: r1149675 - in /subversion/trunk/subversion:
libsvn_repos/hooks.c libsvn_subr/io.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Lieven Govaerts wrote on Fri, Jul 22, 2011 at 20:50:07 +0200:
> On Fri, Jul 22, 2011 at 8:06 PM, <da...@apache.org> wrote:
> > Author: danielsh
> > Date: Fri Jul 22 18:06:12 2011
> > New Revision: 1149675
> >
> > URL: http://svn.apache.org/viewvc?rev=1149675&view=rev
> > Log:
> > Add error checking. This manifested as a segfault and a file descriptor leak
> > in svnsync of an ra_serf source to an ra_local target.
> >
>
> Just to be clear on the wording here: this change does not solve the
> file descriptor leak, but does solve the segfault resulting from a
> file descriptor leak, currently still in the code. Right?
>
That's correct.
> Tested the fix with success on Mac OS X.
>
> Lieven
>
> > * subversion/libsvn_subr/io.c
> > (svn_io_start_cmd2): Check the return value of apr_procattr_io_set().
> >
> > * subversion/libsvn_repos/hooks.c
> > (run_hook_cmd): Do not try to use CMD_PROC when it may not have been initialized.
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_repos/hooks.c
> > subversion/trunk/subversion/libsvn_subr/io.c
Re: svn commit: r1149675 - in /subversion/trunk/subversion:
libsvn_repos/hooks.c libsvn_subr/io.c
Posted by Lieven Govaerts <sv...@mobsol.be>.
On Fri, Jul 22, 2011 at 8:06 PM, <da...@apache.org> wrote:
> Author: danielsh
> Date: Fri Jul 22 18:06:12 2011
> New Revision: 1149675
>
> URL: http://svn.apache.org/viewvc?rev=1149675&view=rev
> Log:
> Add error checking. This manifested as a segfault and a file descriptor leak
> in svnsync of an ra_serf source to an ra_local target.
>
Just to be clear on the wording here: this change does not solve the
file descriptor leak, but does solve the segfault resulting from a
file descriptor leak, currently still in the code. Right?
Tested the fix with success on Mac OS X.
Lieven
> * subversion/libsvn_subr/io.c
> (svn_io_start_cmd2): Check the return value of apr_procattr_io_set().
>
> * subversion/libsvn_repos/hooks.c
> (run_hook_cmd): Do not try to use CMD_PROC when it may not have been initialized.
>
> Modified:
> subversion/trunk/subversion/libsvn_repos/hooks.c
> subversion/trunk/subversion/libsvn_subr/io.c
>
> Modified: subversion/trunk/subversion/libsvn_repos/hooks.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/hooks.c?rev=1149675&r1=1149674&r2=1149675&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_repos/hooks.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/hooks.c Fri Jul 22 18:06:12 2011
> @@ -202,7 +202,8 @@ run_hook_cmd(svn_string_t **result,
>
> if (err)
> {
> - err = svn_error_createf
> + /* CMD_PROC is not safe to use. Bail. */
> + return svn_error_createf
> (SVN_ERR_REPOS_HOOK_FAILURE, err, _("Failed to start '%s' hook"), cmd);
> }
> else
>
> Modified: subversion/trunk/subversion/libsvn_subr/io.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/io.c?rev=1149675&r1=1149674&r2=1149675&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/io.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/io.c Fri Jul 22 18:06:12 2011
> @@ -2489,10 +2489,17 @@ svn_io_start_cmd2(apr_proc_t *cmd_proc,
>
> /* Forward request for pipes to APR. */
> if (infile_pipe || outfile_pipe || errfile_pipe)
> - apr_procattr_io_set(cmdproc_attr,
> - infile_pipe ? APR_FULL_BLOCK : APR_NO_PIPE,
> - outfile_pipe ? APR_FULL_BLOCK : APR_NO_PIPE,
> - errfile_pipe ? APR_FULL_BLOCK : APR_NO_PIPE);
> + {
> + apr_err = apr_procattr_io_set(cmdproc_attr,
> + infile_pipe ? APR_FULL_BLOCK : APR_NO_PIPE,
> + outfile_pipe ? APR_FULL_BLOCK : APR_NO_PIPE,
> + errfile_pipe ? APR_FULL_BLOCK : APR_NO_PIPE);
> +
> + if (apr_err)
> + return svn_error_wrap_apr(apr_err,
> + _("Can't set process '%s' stdio pipes"),
> + cmd);
> + }
>
> /* Have the child print any problems executing its program to errfile. */
> apr_err = apr_pool_userdata_set(errfile, ERRFILE_KEY, NULL, pool);
>
>
>