Hyrum K Wright wrote on Sat, Jan 29, 2011 at 12:03:18 -0600:
> On Fri, Jan 28, 2011 at 9:23 PM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> > hwright_at_apache.org wrote on Fri, Jan 28, 2011 at 20:01:35 -0000:
> >> Author: hwright
> >> Date: Fri Jan 28 20:01:35 2011
> >> New Revision: 1064847
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1064847&view=rev
> >> Log:
> >> * subversion/svnserve/serve.c
> >> (log_cmd): Remove a useless check, and replace it with an assert instead.
> >>
> >> Modified:
> >> subversion/trunk/subversion/svnserve/serve.c
> >>
> >> Modified: subversion/trunk/subversion/svnserve/serve.c
> >> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svnserve/serve.c?rev=1064847&r1=1064846&r2=1064847&view=diff
> >> ==============================================================================
> >> --- subversion/trunk/subversion/svnserve/serve.c (original)
> >> +++ subversion/trunk/subversion/svnserve/serve.c Fri Jan 28 20:01:35 2011
> >> @@ -2008,18 +2008,17 @@ static svn_error_t *log_cmd(svn_ra_svn_c
> >> revprops = NULL;
> >> else if (strcmp(revprop_word, "revprops") == 0)
> >> {
> >> + SVN_ERR_ASSERT(revprop_items);
> >> +
> >> - if (revprop_items)
> >
> > <as far as I can tell>
> >
> > The 'protocol' document explicitly allows the tuple to terminate
> > immediately after the 'revprops' word --- which, is the case where the
> > assert would fire; therefore, either your new check violates the
> > documented protocol, or the protocol documentation needs to be fixed.
> >
> > </as far as I can tell>
>
> The protocol document is in error: 'revprops' must always be followed
> by a list, even if it is the empty list, in which case revprop_items
> on the server is initialized correctly. If 'revprops' is not followed
> by a list, the server emits a malformed network data error and closes
> the network connection post haste. I discovered all this by playing
> around with a python script to hit an svnserve instance with various
> combinations of arguments to the log command.
>
> Given the above, I think we can just remove the assert, as it won't
> ever trigger.
>
I see. The server, in spite of the 'protocol' doc, does:
SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "l(?r)(?r)bb?n?Bwl", &paths,
which --- because of the last 'l' isn't optional --- throws an error if
the tuple is omitted, before the assert() is even reached.
We could fix either the documentation or the implementation, but in this
case how about tweaking the code to match the docs? ---
[[[
Index: subversion/svnserve/serve.c
===================================================================
--- subversion/svnserve/serve.c (revision 1064941)
+++ subversion/svnserve/serve.c (working copy)
@@ -1990,7 +1990,7 @@ static svn_error_t *log_cmd(svn_ra_svn_conn_t *con
apr_uint64_t limit, include_merged_revs_param;
log_baton_t lb;
- SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "l(?r)(?r)bb?n?Bwl", &paths,
+ SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "l(?r)(?r)bb?n?Bw?l", &paths,
&start_rev, &end_rev, &changed_paths,
&strict_node, &limit,
&include_merged_revs_param,
@@ -2008,10 +2008,9 @@ static svn_error_t *log_cmd(svn_ra_svn_conn_t *con
revprops = NULL;
else if (strcmp(revprop_word, "revprops") == 0)
{
- SVN_ERR_ASSERT(revprop_items);
+ int nelts = (revprop_items ? revprop_items->nelts : 0);
- revprops = apr_array_make(pool, revprop_items->nelts,
- sizeof(char *));
+ revprops = apr_array_make(pool, nelts, sizeof(char *));
for (i = 0; i < revprop_items->nelts; i++)
{
elt = &APR_ARRAY_IDX(revprop_items, i, svn_ra_svn_item_t);
]]]
Thanks,
Daniel
> -Hyrum
Received on 2011-01-29 21:41:10 CET