You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Evgeny Kotkov <ev...@visualsvn.com> on 2015/02/09 15:05:52 UTC
Re: svn commit: r1483570 - /subversion/trunk/subversion/libsvn_repos/log.c
Stefan Fuhrmann <st...@apache.org> writes:
> Within libsvn_repos get_log functionality, pass the list of wanted revprops
> around as an array of svn_string_t* instead of const char*. The added length
> info allows for more effient functions to be used. Do that.
[...]
> - char *name = APR_ARRAY_IDX(revprops, i, char *);
> - svn_string_t *value = svn_hash_gets(r_props, name);
> - if (censor_revprops
> - && !(strcmp(name, SVN_PROP_REVISION_AUTHOR) == 0
> - || strcmp(name, SVN_PROP_REVISION_DATE) == 0))
> - /* ... but we can only return author/date. */
> - continue;
[...]
> + const svn_string_t *name
> + = APR_ARRAY_IDX(revprops, i, const svn_string_t *);
> + svn_string_t *value
> + = apr_hash_get(r_props, name->data, name->len);
> + if (censor_revprops
> + && !(strncmp(name->data, SVN_PROP_REVISION_AUTHOR,
> + name->len) == 0
> + || strncmp(name->data, SVN_PROP_REVISION_DATE,
> + name->len) == 0))
> + /* ... but we can only return author/date. */
> + continue;
As it turns out, this particular micro-optimization makes a data leak possible.
This is not a real security issue, as the change happened on trunk and didn't
become part of any released version. Still, I think that we should fix this
prior to making 1.9 public.
I don't know what are the performance implications of using strncmp() in favor
of strcmp(), but the new check will not censor properties like 's', 'sv', ...
'svn:a', 'svn:d' and others. This means that we might incorrectly leak these
revision properties for partially visible revisions. Subversion 1.8.x only
outputs svn:date / svn:author when perfoming log requests for partially visible
revisions, and *all* other revision properties are censored out, but with this
changeset this is no longer true.
I committed a failing test in r1658406. As for fixing this issue, I think that
we should entirely revert this changeset.
Thoughts?
Regards,
Evgeny Kotkov
Re: svn commit: r1483570 - /subversion/trunk/subversion/libsvn_repos/log.c
Posted by Branko Čibej <br...@wandisco.com>.
On 09.02.2015 15:56, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: Evgeny Kotkov [mailto:evgeny.kotkov@visualsvn.com]
>> Sent: maandag 9 februari 2015 15:06
>> To: Subversion Development
>> Subject: Re: svn commit: r1483570 -
>> /subversion/trunk/subversion/libsvn_repos/log.c
>>
>> Stefan Fuhrmann <st...@apache.org> writes:
>>
>>> Within libsvn_repos get_log functionality, pass the list of wanted revprops
>>> around as an array of svn_string_t* instead of const char*. The added length
>>> info allows for more effient functions to be used. Do that.
>> [...]
>>
>>> - char *name = APR_ARRAY_IDX(revprops, i, char *);
>>> - svn_string_t *value = svn_hash_gets(r_props, name);
>>> - if (censor_revprops
>>> - && !(strcmp(name, SVN_PROP_REVISION_AUTHOR) == 0
>>> - || strcmp(name, SVN_PROP_REVISION_DATE) == 0))
>>> - /* ... but we can only return author/date. */
>>> - continue;
>> [...]
>>
>>> + const svn_string_t *name
>>> + = APR_ARRAY_IDX(revprops, i, const svn_string_t *);
>>> + svn_string_t *value
>>> + = apr_hash_get(r_props, name->data, name->len);
>>> + if (censor_revprops
>>> + && !(strncmp(name->data, SVN_PROP_REVISION_AUTHOR,
>>> + name->len) == 0
>>> + || strncmp(name->data, SVN_PROP_REVISION_DATE,
>>> + name->len) == 0))
>>> + /* ... but we can only return author/date. */
>>> + continue;
>> As it turns out, this particular micro-optimization makes a data leak possible.
>> This is not a real security issue, as the change happened on trunk and didn't
>> become part of any released version. Still, I think that we should fix this
>> prior to making 1.9 public.
>>
>> I don't know what are the performance implications of using strncmp() in favor
>> of strcmp(), but the new check will not censor properties like 's', 'sv', ...
>> 'svn:a', 'svn:d' and others. This means that we might incorrectly leak these
>> revision properties for partially visible revisions. Subversion 1.8.x only
>> outputs svn:date / svn:author when perfoming log requests for partially visible
>> revisions, and *all* other revision properties are censored out, but with this
>> changeset this is no longer true.
>>
>> I committed a failing test in r1658406. As for fixing this issue, I think that
>> we should entirely revert this changeset.
> +1
+1; I'm tired of micro-optimizations without proper proof of performance
comparisons that also break code.
-- Brane
RE: svn commit: r1483570 - /subversion/trunk/subversion/libsvn_repos/log.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Evgeny Kotkov [mailto:evgeny.kotkov@visualsvn.com]
> Sent: maandag 9 februari 2015 15:06
> To: Subversion Development
> Subject: Re: svn commit: r1483570 -
> /subversion/trunk/subversion/libsvn_repos/log.c
>
> Stefan Fuhrmann <st...@apache.org> writes:
>
> > Within libsvn_repos get_log functionality, pass the list of wanted revprops
> > around as an array of svn_string_t* instead of const char*. The added length
> > info allows for more effient functions to be used. Do that.
>
> [...]
>
> > - char *name = APR_ARRAY_IDX(revprops, i, char *);
> > - svn_string_t *value = svn_hash_gets(r_props, name);
> > - if (censor_revprops
> > - && !(strcmp(name, SVN_PROP_REVISION_AUTHOR) == 0
> > - || strcmp(name, SVN_PROP_REVISION_DATE) == 0))
> > - /* ... but we can only return author/date. */
> > - continue;
>
> [...]
>
> > + const svn_string_t *name
> > + = APR_ARRAY_IDX(revprops, i, const svn_string_t *);
> > + svn_string_t *value
> > + = apr_hash_get(r_props, name->data, name->len);
> > + if (censor_revprops
> > + && !(strncmp(name->data, SVN_PROP_REVISION_AUTHOR,
> > + name->len) == 0
> > + || strncmp(name->data, SVN_PROP_REVISION_DATE,
> > + name->len) == 0))
> > + /* ... but we can only return author/date. */
> > + continue;
>
> As it turns out, this particular micro-optimization makes a data leak possible.
> This is not a real security issue, as the change happened on trunk and didn't
> become part of any released version. Still, I think that we should fix this
> prior to making 1.9 public.
>
> I don't know what are the performance implications of using strncmp() in favor
> of strcmp(), but the new check will not censor properties like 's', 'sv', ...
> 'svn:a', 'svn:d' and others. This means that we might incorrectly leak these
> revision properties for partially visible revisions. Subversion 1.8.x only
> outputs svn:date / svn:author when perfoming log requests for partially visible
> revisions, and *all* other revision properties are censored out, but with this
> changeset this is no longer true.
>
> I committed a failing test in r1658406. As for fixing this issue, I think that
> we should entirely revert this changeset.
+1
Bert
Re: svn commit: r1483570 - /subversion/trunk/subversion/libsvn_repos/log.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Feb 9, 2015 at 4:38 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 09.02.2015 16:34, Stefan Fuhrmann wrote:
> > On Mon, Feb 9, 2015 at 3:05 PM, Evgeny Kotkov
> > <evgeny.kotkov@visualsvn.com <ma...@visualsvn.com>>
> wrote:
> >
> > I committed a failing test in r1658406. As for fixing this issue,
> > I think that
> > we should entirely revert this changeset.
> >
> >
> > Authz performance has always been a sore spot.
> > I think making hurt less is worth some code.
>
> But we're doing that on the authzperf branch, where the benefits are
> actually really tangible. Not on trunk while we're trying to stabilize
> it for the release and without at least minimal performance comparison
> on real-life data.
>
That branch did not exist in Mai 2013, when the bug got introduced.
-- Stefan^2.
Re: svn commit: r1483570 - /subversion/trunk/subversion/libsvn_repos/log.c
Posted by Branko Čibej <br...@wandisco.com>.
On 09.02.2015 16:34, Stefan Fuhrmann wrote:
> On Mon, Feb 9, 2015 at 3:05 PM, Evgeny Kotkov
> <evgeny.kotkov@visualsvn.com <ma...@visualsvn.com>> wrote:
>
> I committed a failing test in r1658406. As for fixing this issue,
> I think that
> we should entirely revert this changeset.
>
>
> Authz performance has always been a sore spot.
> I think making hurt less is worth some code.
But we're doing that on the authzperf branch, where the benefits are
actually really tangible. Not on trunk while we're trying to stabilize
it for the release and without at least minimal performance comparison
on real-life data.
-- Brane
Re: svn commit: r1483570 - /subversion/trunk/subversion/libsvn_repos/log.c
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Stefan Fuhrmann <st...@wandisco.com> writes:
> I had two basic options to fix the bug.
> (1) Use svn_string_compare that is there for exactly that purpose.
> (2) Fall back to low-level C-string ops and hope to get it right this time.
>
> I chose with option (1). We use the "sizeof(define) -1" construct
> in at least 32 other places already. I simply went the extra mile
> and documented it, which makes it look like something out of
> the ordinary.
Indeed, this huge comment in r1658439 caught my attention, and I thought that
it wasn't something we do a lot. Grepping gives more than a hundred hits on
usages like this one, so I am fine with that.
>> By the way, it looks like this changeset [1] did not really remove the
>> XFail() marker from the new test, so I expect the buildbots to turn red.
>
> Yes, I noticed that and fixed it in r1658442.
Thank you for fixing this and the problem itself.
Regards,
Evgeny Kotkov.
Re: svn commit: r1483570 - /subversion/trunk/subversion/libsvn_repos/log.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Feb 9, 2015 at 4:52 PM, Evgeny Kotkov <ev...@visualsvn.com>
wrote:
> Stefan Fuhrmann <st...@apache.org> writes:
>
> >> As it turns out, this particular micro-optimization makes a data leak
> >> possible. This is not a real security issue, as the change happened on
> >> trunk and didn't become part of any released version. Still, I think
> >> that we should fix this prior to making 1.9 public.
> >
> > Good catch, Evgeny! Fixed in r1658439.
>
> I wonder if we really should be adding static const svn_string_t's, and
> using
> "sizeof(define) -1" for unknown performance improvements without
> appropriate
> measurements.
I had two basic options to fix the bug.
(1) Use svn_string_compare that is there for exactly that purpose.
(2) Fall back to low-level C-string ops and hope to get it right this time.
I chose with option (1). We use the "sizeof(define) -1" construct
in at least 32 other places already. I simply went the extra mile
and documented it, which makes it look like something out of
the ordinary.
> By the way, it looks like this changeset [1] did not really
> remove the XFail() marker from the new test, so I expect the buildbots to
> turn red.
>
Yes, I noticed that and fixed it in r1658442.
-- Stefan^2.
Re: svn commit: r1483570 - /subversion/trunk/subversion/libsvn_repos/log.c
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Stefan Fuhrmann <st...@apache.org> writes:
>> As it turns out, this particular micro-optimization makes a data leak
>> possible. This is not a real security issue, as the change happened on
>> trunk and didn't become part of any released version. Still, I think
>> that we should fix this prior to making 1.9 public.
>
> Good catch, Evgeny! Fixed in r1658439.
I wonder if we really should be adding static const svn_string_t's, and using
"sizeof(define) -1" for unknown performance improvements without appropriate
measurements. By the way, it looks like this changeset [1] did not really
remove the XFail() marker from the new test, so I expect the buildbots to
turn red.
[1] http://svn.apache.org/r1658439
Regards,
Evgeny Kotkov
Re: svn commit: r1483570 - /subversion/trunk/subversion/libsvn_repos/log.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Feb 9, 2015 at 3:05 PM, Evgeny Kotkov <ev...@visualsvn.com>
wrote:
> Stefan Fuhrmann <st...@apache.org> writes:
>
> > Within libsvn_repos get_log functionality, pass the list of wanted
> revprops
> > around as an array of svn_string_t* instead of const char*. The added
> length
> > info allows for more effient functions to be used. Do that.
>
> [...]
>
> > - char *name = APR_ARRAY_IDX(revprops, i, char *);
> > - svn_string_t *value = svn_hash_gets(r_props, name);
> > - if (censor_revprops
> > - && !(strcmp(name, SVN_PROP_REVISION_AUTHOR) == 0
> > - || strcmp(name, SVN_PROP_REVISION_DATE) == 0))
> > - /* ... but we can only return author/date. */
> > - continue;
>
> [...]
>
> > + const svn_string_t *name
> > + = APR_ARRAY_IDX(revprops, i, const svn_string_t *);
> > + svn_string_t *value
> > + = apr_hash_get(r_props, name->data, name->len);
> > + if (censor_revprops
> > + && !(strncmp(name->data, SVN_PROP_REVISION_AUTHOR,
> > + name->len) == 0
> > + || strncmp(name->data, SVN_PROP_REVISION_DATE,
> > + name->len) == 0))
> > + /* ... but we can only return author/date. */
> > + continue;
>
> As it turns out, this particular micro-optimization makes a data leak
> possible.
> This is not a real security issue, as the change happened on trunk and
> didn't
> become part of any released version. Still, I think that we should fix
> this
> prior to making 1.9 public.
>
Good catch, Evgeny! Fixed in r1658439.
> I don't know what are the performance implications of using strncmp() in
> favor
> of strcmp(),
No that is a simple thinko. The actual optimization
happened in the block before the fallback code block
that introduced the error. With the fix, we now use
proper and fast string comparison even in that code
block.
> but the new check will not censor properties like 's', 'sv', ...
> 'svn:a', 'svn:d' and others. This means that we might incorrectly leak
> these
> revision properties for partially visible revisions. Subversion 1.8.x only
> outputs svn:date / svn:author when perfoming log requests for partially
> visible
> revisions, and *all* other revision properties are censored out, but with
> this
> changeset this is no longer true.
>
> I committed a failing test in r1658406. As for fixing this issue, I think
> that
> we should entirely revert this changeset.
>
Authz performance has always been a sore spot.
I think making hurt less is worth some code.
-- Stefan^2.