Daniel Shahaf wrote on Sat, Jan 29, 2011 at 22:36:29 +0200:
> 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...@daniel.shahaf.name>
> > wrote:
> > > hwri...@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++)
+ for (i = 0; i < nelts; i++)
> {
> elt = &APR_ARRAY_IDX(revprop_items, i, svn_ra_svn_item_t);
> ]]]
>
> Thanks,
>
> Daniel
>
> > -Hyrum
