You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2011/07/08 19:55:16 UTC
Re: svn commit: r1144316 - in
/subversion/trunk/subversion/libsvn_repos: commit.c fs-wrap.c
philip@apache.org wrote on Fri, Jul 08, 2011 at 14:02:42 -0000:
> Author: philip
> Date: Fri Jul 8 14:02:42 2011
> New Revision: 1144316
>
> URL: http://svn.apache.org/viewvc?rev=1144316&view=rev
> Log:
> Fix issue 3953, mod_dav_svn failing to reject bogus mergeinfo on commit.
> Move server-side mergeinfo validation so that all RA layers use it.
>
...
> @@ -222,6 +255,9 @@ svn_repos_fs_change_node_prop(svn_fs_roo
> const svn_string_t *value,
> apr_pool_t *pool)
> {
> + if (value && strcmp(name, SVN_PROP_MERGEINFO) == 0)
> + SVN_ERR(verify_mergeinfo(value, path, pool));
> +
> /* Validate the property, then call the wrapped function. */
> SVN_ERR(svn_repos__validate_prop(name, value, pool));
> return svn_fs_change_node_prop(root, path, name, value, pool);
>
Shouldn't the call to verify_mergeinfo() be made by
svn_repos__validate_prop(), rather than here?
>
Re: svn commit: r1144316 - in
/subversion/trunk/subversion/libsvn_repos: commit.c fs-wrap.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Fri, Jul 08, 2011 at 20:39:02 +0100:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> > My point wasn't the validation of svn:mergeinfo revprops, it was whether
> > the validation of svn:mergeinfo nodeprops should happen in
> > svn_repos__validate_prop().
> >
> > I'm not against telling that function whether it's validating a nodeprop
> > or a revprop, and applying different logic in either case.
>
> Is there any common validation between node props and rev props? If the
> validation is completely different it should be two functions rather
> than one function with a boolean flag.
>
These two hunks seem to be generic:
/* Disallow setting non-regular properties. */
if (kind != svn_prop_regular_kind)
return svn_error_createf
(SVN_ERR_REPOS_BAD_ARGS, NULL,
_("Storage of non-regular property '%s' is disallowed through the "
"repository interface, and could indicate a bug in your client"),
name);
/* Validate that translated props (e.g., svn:log) are UTF-8 with
* LF line endings. */
if (svn_prop_needs_translation(name))
{
if (svn_utf__is_valid(value->data, value->len) == FALSE)
{
return svn_error_createf
(SVN_ERR_BAD_PROPERTY_VALUE, NULL,
_("Cannot accept '%s' property because it is not encoded in "
"UTF-8"), name);
}
/* Disallow inconsistent line ending style, by simply looking for
* carriage return characters ('\r'). */
if (strchr(value->data, '\r') != NULL)
{
return svn_error_createf
(SVN_ERR_BAD_PROPERTY_VALUE, NULL,
_("Cannot accept non-LF line endings in '%s' property"),
name);
}
}
But, might as well split those two to a helper function that both
__validate_revprop() and __validate_nodeprop() call.
In short: not too worried. Either way is fine for now (these ARE
private API's); let's get it done rather than debate the form.
> --
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com
Re: svn commit: r1144316 - in /subversion/trunk/subversion/libsvn_repos: commit.c fs-wrap.c
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> My point wasn't the validation of svn:mergeinfo revprops, it was whether
> the validation of svn:mergeinfo nodeprops should happen in
> svn_repos__validate_prop().
>
> I'm not against telling that function whether it's validating a nodeprop
> or a revprop, and applying different logic in either case.
Is there any common validation between node props and rev props? If the
validation is completely different it should be two functions rather
than one function with a boolean flag.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1144316 - in
/subversion/trunk/subversion/libsvn_repos: commit.c fs-wrap.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
My point wasn't the validation of svn:mergeinfo revprops, it was whether
the validation of svn:mergeinfo nodeprops should happen in
svn_repos__validate_prop().
I'm not against telling that function whether it's validating a nodeprop
or a revprop, and applying different logic in either case.
Philip Martin wrote on Fri, Jul 08, 2011 at 20:03:48 +0100:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> >> Perhaps. svn_repos__validate_prop is also used by
> >> svn_repos_fs_change_txn_props, is it appropriate to validate mergeinfo
> >> there?
> >>
> >
> > Why not? No one should be setting svn:mergeinfo as a txnprop (or
> > revprop). (and if they do, they shouldn't use the svn:* namespace)
> >
> >> We should probably split the validate function into two, one for node
> >> props and one for revprops.
> >>
> >
> > Given that we don't have any SVN_PROP_* whose semantics differ when it's
> > set as a revprop v. when it's set as a nodeprop, I'm not sure what this
> > would gain; I'm ±0.
>
> Huh? They differ totally. It makes no sense to check svn:mergeinfo
> syntax valid set as a revprop, we should either allow all values or
> none.
>
> > Unless, perhaps, you want to add verification that svn:mergeinfo isn't
> > set as a revprop and svn:log isn't set as a nodeprop? Feel free to do
> > so, but then I suggest you'll also teach svnsync et al to strip those
> > (ill-set) properties to avoid breaking any repositories out there that
> > do have svn:mergeinfo revprops and svn:log nodeprops.
>
> Old repositories are already problem. The existing svn:mergeinfo
> validation is going to prevent people dumping/loading repositories with
> invalid svn:mergeinfo on nodes. We don't want to add to the problem by
> adding syntax checking where we don't need it unless we *also* add the
> stripping code.
>
> --
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com
Re: svn commit: r1144316 - in
/subversion/trunk/subversion/libsvn_repos: commit.c fs-wrap.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
My point wasn't the validation of svn:mergeinfo revprops, it was whether
the validation of svn:mergeinfo nodeprops should happen in
svn_repos__validate_prop().
I'm not against telling that function whether it's validating a nodeprop
or a revprop, and applying different logic in either case.
Philip Martin wrote on Fri, Jul 08, 2011 at 20:03:48 +0100:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> >> Perhaps. svn_repos__validate_prop is also used by
> >> svn_repos_fs_change_txn_props, is it appropriate to validate mergeinfo
> >> there?
> >>
> >
> > Why not? No one should be setting svn:mergeinfo as a txnprop (or
> > revprop). (and if they do, they shouldn't use the svn:* namespace)
> >
> >> We should probably split the validate function into two, one for node
> >> props and one for revprops.
> >>
> >
> > Given that we don't have any SVN_PROP_* whose semantics differ when it's
> > set as a revprop v. when it's set as a nodeprop, I'm not sure what this
> > would gain; I'm ±0.
>
> Huh? They differ totally. It makes no sense to check svn:mergeinfo
> syntax valid set as a revprop, we should either allow all values or
> none.
>
> > Unless, perhaps, you want to add verification that svn:mergeinfo isn't
> > set as a revprop and svn:log isn't set as a nodeprop? Feel free to do
> > so, but then I suggest you'll also teach svnsync et al to strip those
> > (ill-set) properties to avoid breaking any repositories out there that
> > do have svn:mergeinfo revprops and svn:log nodeprops.
>
> Old repositories are already problem. The existing svn:mergeinfo
> validation is going to prevent people dumping/loading repositories with
> invalid svn:mergeinfo on nodes. We don't want to add to the problem by
> adding syntax checking where we don't need it unless we *also* add the
> stripping code.
>
> --
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com
Re: svn commit: r1144316 - in /subversion/trunk/subversion/libsvn_repos: commit.c fs-wrap.c
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
>> Perhaps. svn_repos__validate_prop is also used by
>> svn_repos_fs_change_txn_props, is it appropriate to validate mergeinfo
>> there?
>>
>
> Why not? No one should be setting svn:mergeinfo as a txnprop (or
> revprop). (and if they do, they shouldn't use the svn:* namespace)
>
>> We should probably split the validate function into two, one for node
>> props and one for revprops.
>>
>
> Given that we don't have any SVN_PROP_* whose semantics differ when it's
> set as a revprop v. when it's set as a nodeprop, I'm not sure what this
> would gain; I'm ±0.
Huh? They differ totally. It makes no sense to check svn:mergeinfo
syntax valid set as a revprop, we should either allow all values or
none.
> Unless, perhaps, you want to add verification that svn:mergeinfo isn't
> set as a revprop and svn:log isn't set as a nodeprop? Feel free to do
> so, but then I suggest you'll also teach svnsync et al to strip those
> (ill-set) properties to avoid breaking any repositories out there that
> do have svn:mergeinfo revprops and svn:log nodeprops.
Old repositories are already problem. The existing svn:mergeinfo
validation is going to prevent people dumping/loading repositories with
invalid svn:mergeinfo on nodes. We don't want to add to the problem by
adding syntax checking where we don't need it unless we *also* add the
stripping code.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1144316 - in /subversion/trunk/subversion/libsvn_repos: commit.c fs-wrap.c
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
>> Perhaps. svn_repos__validate_prop is also used by
>> svn_repos_fs_change_txn_props, is it appropriate to validate mergeinfo
>> there?
>>
>
> Why not? No one should be setting svn:mergeinfo as a txnprop (or
> revprop). (and if they do, they shouldn't use the svn:* namespace)
>
>> We should probably split the validate function into two, one for node
>> props and one for revprops.
>>
>
> Given that we don't have any SVN_PROP_* whose semantics differ when it's
> set as a revprop v. when it's set as a nodeprop, I'm not sure what this
> would gain; I'm ±0.
Huh? They differ totally. It makes no sense to check svn:mergeinfo
syntax valid set as a revprop, we should either allow all values or
none.
> Unless, perhaps, you want to add verification that svn:mergeinfo isn't
> set as a revprop and svn:log isn't set as a nodeprop? Feel free to do
> so, but then I suggest you'll also teach svnsync et al to strip those
> (ill-set) properties to avoid breaking any repositories out there that
> do have svn:mergeinfo revprops and svn:log nodeprops.
Old repositories are already problem. The existing svn:mergeinfo
validation is going to prevent people dumping/loading repositories with
invalid svn:mergeinfo on nodes. We don't want to add to the problem by
adding syntax checking where we don't need it unless we *also* add the
stripping code.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1144316 - in
/subversion/trunk/subversion/libsvn_repos: commit.c fs-wrap.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Fri, Jul 08, 2011 at 19:30:02 +0100:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> > philip@apache.org wrote on Fri, Jul 08, 2011 at 14:02:42 -0000:
> >> Author: philip
> >> Date: Fri Jul 8 14:02:42 2011
> >> New Revision: 1144316
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1144316&view=rev
> >> Log:
> >> Fix issue 3953, mod_dav_svn failing to reject bogus mergeinfo on commit.
> >> Move server-side mergeinfo validation so that all RA layers use it.
> >>
> > ...
> >> @@ -222,6 +255,9 @@ svn_repos_fs_change_node_prop(svn_fs_roo
> >> const svn_string_t *value,
> >> apr_pool_t *pool)
> >> {
> >> + if (value && strcmp(name, SVN_PROP_MERGEINFO) == 0)
> >> + SVN_ERR(verify_mergeinfo(value, path, pool));
> >> +
> >> /* Validate the property, then call the wrapped function. */
> >> SVN_ERR(svn_repos__validate_prop(name, value, pool));
> >> return svn_fs_change_node_prop(root, path, name, value, pool);
> >>
> >
> > Shouldn't the call to verify_mergeinfo() be made by
> > svn_repos__validate_prop(), rather than here?
>
> Perhaps. svn_repos__validate_prop is also used by
> svn_repos_fs_change_txn_props, is it appropriate to validate mergeinfo
> there?
>
Why not? No one should be setting svn:mergeinfo as a txnprop (or
revprop). (and if they do, they shouldn't use the svn:* namespace)
> We should probably split the validate function into two, one for node
> props and one for revprops.
>
Given that we don't have any SVN_PROP_* whose semantics differ when it's
set as a revprop v. when it's set as a nodeprop, I'm not sure what this
would gain; I'm ±0.
Unless, perhaps, you want to add verification that svn:mergeinfo isn't
set as a revprop and svn:log isn't set as a nodeprop? Feel free to do
so, but then I suggest you'll also teach svnsync et al to strip those
(ill-set) properties to avoid breaking any repositories out there that
do have svn:mergeinfo revprops and svn:log nodeprops.
> --
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com
Re: svn commit: r1144316 - in
/subversion/trunk/subversion/libsvn_repos: commit.c fs-wrap.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Fri, Jul 08, 2011 at 19:30:02 +0100:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> > philip@apache.org wrote on Fri, Jul 08, 2011 at 14:02:42 -0000:
> >> Author: philip
> >> Date: Fri Jul 8 14:02:42 2011
> >> New Revision: 1144316
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1144316&view=rev
> >> Log:
> >> Fix issue 3953, mod_dav_svn failing to reject bogus mergeinfo on commit.
> >> Move server-side mergeinfo validation so that all RA layers use it.
> >>
> > ...
> >> @@ -222,6 +255,9 @@ svn_repos_fs_change_node_prop(svn_fs_roo
> >> const svn_string_t *value,
> >> apr_pool_t *pool)
> >> {
> >> + if (value && strcmp(name, SVN_PROP_MERGEINFO) == 0)
> >> + SVN_ERR(verify_mergeinfo(value, path, pool));
> >> +
> >> /* Validate the property, then call the wrapped function. */
> >> SVN_ERR(svn_repos__validate_prop(name, value, pool));
> >> return svn_fs_change_node_prop(root, path, name, value, pool);
> >>
> >
> > Shouldn't the call to verify_mergeinfo() be made by
> > svn_repos__validate_prop(), rather than here?
>
> Perhaps. svn_repos__validate_prop is also used by
> svn_repos_fs_change_txn_props, is it appropriate to validate mergeinfo
> there?
>
Why not? No one should be setting svn:mergeinfo as a txnprop (or
revprop). (and if they do, they shouldn't use the svn:* namespace)
> We should probably split the validate function into two, one for node
> props and one for revprops.
>
Given that we don't have any SVN_PROP_* whose semantics differ when it's
set as a revprop v. when it's set as a nodeprop, I'm not sure what this
would gain; I'm ±0.
Unless, perhaps, you want to add verification that svn:mergeinfo isn't
set as a revprop and svn:log isn't set as a nodeprop? Feel free to do
so, but then I suggest you'll also teach svnsync et al to strip those
(ill-set) properties to avoid breaking any repositories out there that
do have svn:mergeinfo revprops and svn:log nodeprops.
> --
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com
Re: svn commit: r1144316 - in /subversion/trunk/subversion/libsvn_repos: commit.c fs-wrap.c
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> philip@apache.org wrote on Fri, Jul 08, 2011 at 14:02:42 -0000:
>> Author: philip
>> Date: Fri Jul 8 14:02:42 2011
>> New Revision: 1144316
>>
>> URL: http://svn.apache.org/viewvc?rev=1144316&view=rev
>> Log:
>> Fix issue 3953, mod_dav_svn failing to reject bogus mergeinfo on commit.
>> Move server-side mergeinfo validation so that all RA layers use it.
>>
> ...
>> @@ -222,6 +255,9 @@ svn_repos_fs_change_node_prop(svn_fs_roo
>> const svn_string_t *value,
>> apr_pool_t *pool)
>> {
>> + if (value && strcmp(name, SVN_PROP_MERGEINFO) == 0)
>> + SVN_ERR(verify_mergeinfo(value, path, pool));
>> +
>> /* Validate the property, then call the wrapped function. */
>> SVN_ERR(svn_repos__validate_prop(name, value, pool));
>> return svn_fs_change_node_prop(root, path, name, value, pool);
>>
>
> Shouldn't the call to verify_mergeinfo() be made by
> svn_repos__validate_prop(), rather than here?
Perhaps. svn_repos__validate_prop is also used by
svn_repos_fs_change_txn_props, is it appropriate to validate mergeinfo
there?
We should probably split the validate function into two, one for node
props and one for revprops.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Re: svn commit: r1144316 - in /subversion/trunk/subversion/libsvn_repos: commit.c fs-wrap.c
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> philip@apache.org wrote on Fri, Jul 08, 2011 at 14:02:42 -0000:
>> Author: philip
>> Date: Fri Jul 8 14:02:42 2011
>> New Revision: 1144316
>>
>> URL: http://svn.apache.org/viewvc?rev=1144316&view=rev
>> Log:
>> Fix issue 3953, mod_dav_svn failing to reject bogus mergeinfo on commit.
>> Move server-side mergeinfo validation so that all RA layers use it.
>>
> ...
>> @@ -222,6 +255,9 @@ svn_repos_fs_change_node_prop(svn_fs_roo
>> const svn_string_t *value,
>> apr_pool_t *pool)
>> {
>> + if (value && strcmp(name, SVN_PROP_MERGEINFO) == 0)
>> + SVN_ERR(verify_mergeinfo(value, path, pool));
>> +
>> /* Validate the property, then call the wrapped function. */
>> SVN_ERR(svn_repos__validate_prop(name, value, pool));
>> return svn_fs_change_node_prop(root, path, name, value, pool);
>>
>
> Shouldn't the call to verify_mergeinfo() be made by
> svn_repos__validate_prop(), rather than here?
Perhaps. svn_repos__validate_prop is also used by
svn_repos_fs_change_txn_props, is it appropriate to validate mergeinfo
there?
We should probably split the validate function into two, one for node
props and one for revprops.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com