You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by hw...@apache.org on 2011/01/28 21:01:35 UTC
svn commit: r1064847 - /subversion/trunk/subversion/svnserve/serve.c
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);
+
revprops = apr_array_make(pool, revprop_items->nelts,
sizeof(char *));
- if (revprop_items)
+ for (i = 0; i < revprop_items->nelts; i++)
{
- for (i = 0; i < revprop_items->nelts; i++)
- {
- elt = &APR_ARRAY_IDX(revprop_items, i, svn_ra_svn_item_t);
- if (elt->kind != SVN_RA_SVN_STRING)
- return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
- _("Log revprop entry not a string"));
- APR_ARRAY_PUSH(revprops, const char *) = elt->u.string->data;
- }
+ elt = &APR_ARRAY_IDX(revprop_items, i, svn_ra_svn_item_t);
+ if (elt->kind != SVN_RA_SVN_STRING)
+ return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
+ _("Log revprop entry not a string"));
+ APR_ARRAY_PUSH(revprops, const char *) = elt->u.string->data;
}
}
else
Re: svn commit: r1064847 - /subversion/trunk/subversion/svnserve/serve.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Fri, Jan 28, 2011 at 9:23 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> hwright@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>
>
> Makes sense?
I guess.
The diff doesn't give much context, but the prior code was:
revprops = apr_array_make(pool, revprop_items->nelts,
sizeof(char *));
if (revprop_items)
{
...
}
Do you see the problem? The first statement *assumes* that
revprop_items isn't NULL (since it dereferences it), and then goes on
to check this same fact. In practice, I don't think we're ever see
this segfault, but it's a possibility (and may even be a remove DoS
bug if triggered correctly).
r1064847 doesn't change the fact that misbehavior still crashes the
server; it just makes that fact a bit more obvious. Improvements are
welcome.
-Hyrum
Re: svn commit: r1064847 - /subversion/trunk/subversion/svnserve/serve.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Fri, Jan 28, 2011 at 9:23 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> hwright@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>
>
> Makes sense?
I guess.
The diff doesn't give much context, but the prior code was:
revprops = apr_array_make(pool, revprop_items->nelts,
sizeof(char *));
if (revprop_items)
{
...
}
Do you see the problem? The first statement *assumes* that
revprop_items isn't NULL (since it dereferences it), and then goes on
to check this same fact. In practice, I don't think we're ever see
this segfault, but it's a possibility (and may even be a remove DoS
bug if triggered correctly).
r1064847 doesn't change the fact that misbehavior still crashes the
server; it just makes that fact a bit more obvious. Improvements are
welcome.
-Hyrum
Re: svn commit: r1064847 -
/subversion/trunk/subversion/svnserve/serve.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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:
> > > hwright@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
Re: svn commit: r1064847 - /subversion/trunk/subversion/svnserve/serve.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Sat, Jan 29, 2011 at 3:02 PM, Branko Čibej <br...@xbc.nu> wrote:
> On 29.01.2011 21:36, Daniel Shahaf wrote:
>> Hyrum K Wright wrote on Sat, Jan 29, 2011 at 12:03:18 -0600:
>>> 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? ---
>
> Wouldn't it be more appropriate to fix the docs to match the code?
> That's unless that particular difference is a result of a recent change
> (I can hardly believe it is).
This isn't a recent change in behavior, and every 1.6 server out there
expects to have the revprop list. I've updated the docs in r1065715.
-Hyrum
Re: svn commit: r1064847 - /subversion/trunk/subversion/svnserve/serve.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Sat, Jan 29, 2011 at 3:02 PM, Branko Čibej <br...@xbc.nu> wrote:
> On 29.01.2011 21:36, Daniel Shahaf wrote:
>> Hyrum K Wright wrote on Sat, Jan 29, 2011 at 12:03:18 -0600:
>>> 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? ---
>
> Wouldn't it be more appropriate to fix the docs to match the code?
> That's unless that particular difference is a result of a recent change
> (I can hardly believe it is).
This isn't a recent change in behavior, and every 1.6 server out there
expects to have the revprop list. I've updated the docs in r1065715.
-Hyrum
Re: svn commit: r1064847 - /subversion/trunk/subversion/svnserve/serve.c
Posted by Branko Čibej <br...@xbc.nu>.
On 29.01.2011 21:36, Daniel Shahaf wrote:
> Hyrum K Wright wrote on Sat, Jan 29, 2011 at 12:03:18 -0600:
>> 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? ---
Wouldn't it be more appropriate to fix the docs to match the code?
That's unless that particular difference is a result of a recent change
(I can hardly believe it is).
-- Brane
Re: svn commit: r1064847 -
/subversion/trunk/subversion/svnserve/serve.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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:
> > > hwright@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
Re: svn commit: r1064847 -
/subversion/trunk/subversion/svnserve/serve.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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:
> > hwright@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
Re: svn commit: r1064847 -
/subversion/trunk/subversion/svnserve/serve.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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:
> > hwright@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
Re: svn commit: r1064847 - /subversion/trunk/subversion/svnserve/serve.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Fri, Jan 28, 2011 at 9:23 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> hwright@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.
-Hyrum
Re: svn commit: r1064847 - /subversion/trunk/subversion/svnserve/serve.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Fri, Jan 28, 2011 at 9:23 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> hwright@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.
-Hyrum
Re: svn commit: r1064847 - /subversion/trunk/subversion/svnserve/serve.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Sat, Jan 29, 2011 at 8:20 AM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>> Sent: zaterdag 29 januari 2011 4:24
>> To: dev@subversion.apache.org
>> Cc: commits@subversion.apache.org
>> Subject: Re: svn commit: r1064847 -
>> /subversion/trunk/subversion/svnserve/serve.c
>>
>> hwright@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>
>
> And an assertion crashes svnserve, so I would recommend not creating network
> triggerable assertions (in non-maintainer builds) anyway.
Understood. But I didn't create the crash; it was already there as a
potential NULL pointer dereference. The same conditions which would
cause the new assert to fail would have led to a segfault in the old
code. Functionality-wise, I neither improved nor worsened the
situation. My goal was to illuminate the potential crash; I'd say
that's working just fine. :)
(And yes, I do think a more permanent fix is needed, which could be as
simple as returning an MALFORMED error. I haven't thought too much
about it.)
-Hyrum
RE: svn commit: r1064847 - /subversion/trunk/subversion/svnserve/serve.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> Sent: zaterdag 29 januari 2011 4:24
> To: dev@subversion.apache.org
> Cc: commits@subversion.apache.org
> Subject: Re: svn commit: r1064847 -
> /subversion/trunk/subversion/svnserve/serve.c
>
> hwright@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>
And an assertion crashes svnserve, so I would recommend not creating network
triggerable assertions (in non-maintainer builds) anyway.
Bert
>
> Makes sense?
Re: svn commit: r1064847 -
/subversion/trunk/subversion/svnserve/serve.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
hwright@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>
Makes sense?
Re: svn commit: r1064847 -
/subversion/trunk/subversion/svnserve/serve.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
hwright@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>
Makes sense?
Re: svn commit: r1064847 -
/subversion/trunk/subversion/svnserve/serve.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
hwright@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 this is an input validation, then it shouldn't be an assert.
Re: svn commit: r1064847 -
/subversion/trunk/subversion/svnserve/serve.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
hwright@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 this is an input validation, then it shouldn't be an assert.