You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Peter N. Lundblad" <pe...@famlundblad.se> on 2006/03/24 20:25:36 UTC
r19004
> Index: subversion/libsvn_repos/hooks.c
> ===================================================================
> --- subversion/libsvn_repos/hooks.c (revision 19003)
> +++ subversion/libsvn_repos/hooks.c (revision 19004)
[...]
> +{
> + const char *script_stderr_utf8 = "";
> + const char **native_args;
> + char buffer[20];
This seems like a rather small buffer size.
> + /* Allocate memory for the native_args string array plus one for
> + * the ending null element. */
> + native_args = apr_palloc(pool, sizeof(char *) * args_arr_size + 1);
^ ^
Missing parens. Above is interpreted as (sizeof(char*) * args_arr_size) + 1.
> + if (child_pid == -1)
> + {
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error spawning process for hook script '%s'",
You're missing _() marks for translation here and in other places, but
if your port doesn't use gettext, then you're just doing us
translators a favour:-)
> + cmd);
> + }
> +
> + /* If there is "APR stdin", read it and then write it to the hook's
> + * stdin pipe. */
> + if (stdin_handle)
> + {
> + while (1)
> + {
> + apr_size_t bytes_read = sizeof(buffer);
> + int wc;
> + svn_error_t *err = svn_io_file_read(stdin_handle, buffer,
> + &bytes_read, pool);
> + if (err && APR_STATUS_IS_EOF(err->apr_err))
> + break;
> +
> + if (err)
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error piping stdin to hook "
> + "script '%s'", cmd);
> +
> + wc = write(stdin_pipe[1], buffer, bytes_read);
> + if (wc == -1)
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error piping stdin to hook "
> + "script '%s'", cmd);
> + }
> +
> + /* Close the write end of the stdin pipe. */
> + if (close(stdin_pipe[1]) == -1)
> + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> + "Error closing write end of stdin pipe "
> + "to hook script '%s'", cmd);
> + }
> +
This is a deadlock if the stderr pipe fills up while you're writing to
the stdin of the hook script. You need to use poll/select to
read/write simultaneously.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r19004
Posted by Paul Burba <pa...@softlanding.com>.
"Peter N. Lundblad" <pe...@famlundblad.se> wrote on 03/24/2006 03:25:36
PM:
> > Index: subversion/libsvn_repos/hooks.c
> > ===================================================================
<snip>
> > + char buffer[20];
>
> This seems like a rather small buffer size.
This size goes back to an IBM code example I used to get me started, so
there is no compelling reason for it on my part. Searching through the
Subversion code base I see a wide variety of buffer sizes used. Is there
any rule of thumb for such things? Any words of wisdom appreciated.
> > + /* Allocate memory for the native_args string array plus one for
> > + * the ending null element. */
> > + native_args = apr_palloc(pool, sizeof(char *) * args_arr_size + 1);
> ^ ^
> Missing parens. Above is interpreted as (sizeof(char*) * args_arr_size)
+ 1.
> You're missing _() marks for translation here and in other places, but
> if your port doesn't use gettext, then you're just doing us
> translators a favour:-)
We don't support translation as of yet on the iSeries port so I removed
all the _() marks per Julian's recommendation to do it all one way or the
other.
> > + cmd);
> > + }
> > +
> > + /* If there is "APR stdin", read it and then write it to the hook's
> > + * stdin pipe. */
> > + if (stdin_handle)
> > + {
> > + while (1)
> > + {
> > + apr_size_t bytes_read = sizeof(buffer);
> > + int wc;
> > + svn_error_t *err = svn_io_file_read(stdin_handle, buffer,
> > + &bytes_read, pool);
> > + if (err && APR_STATUS_IS_EOF(err->apr_err))
> > + break;
> > +
> > + if (err)
> > + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> > + "Error piping stdin to hook "
> > + "script '%s'", cmd);
> > +
> > + wc = write(stdin_pipe[1], buffer, bytes_read);
> > + if (wc == -1)
> > + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> > + "Error piping stdin to hook "
> > + "script '%s'", cmd);
> > + }
> > +
> > + /* Close the write end of the stdin pipe. */
> > + if (close(stdin_pipe[1]) == -1)
> > + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> > + "Error closing write end of stdin
pipe "
> > + "to hook script '%s'", cmd);
> > + }
> > +
>
> This is a deadlock if the stderr pipe fills up while you're writing to
> the stdin of the hook script. You need to use poll/select to
> read/write simultaneously.
Sorry I committed this before you caught it. The deadlock scenario you
describe was easy to create, even if my example was a bit contrived. I'm
working on this now...more soon.
Paul B.
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org