You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ko...@apache.org on 2015/12/06 13:02:00 UTC
svn commit: r1718167 -
/subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c
Author: kotkov
Date: Sun Dec 6 12:02:00 2015
New Revision: 1718167
URL: http://svn.apache.org/viewvc?rev=1718167&view=rev
Log:
Disable zero-copy code path for ra_local update reporters.
The known and documented limitation of svn_repos_begin_report3() is that
with zero-copy enabled, the delta editor callbacks cannot access FSFS
or use Subversion caches directly. This limitation comes from the fact
that sending delta using the zero-copy code path happens from within a cache
access wrapper — that is, while holding the lock. If a particular delta
consumer happens to access or invalidate the cache, bad things could happen,
spanning from UB due to accessing a dangling pointer to a deadlock caused by
an attempt to take a non-recursive (blocking) lock, that has already been
taken by the same thread.
Within ra_local, we cannot be sure that arbitrary callers of our public
API, namely, svn_ra_do_update3(), svn_ra_do_switch3() or svn_ra_do_status2(),
are aware of this limitation and pass-in the delta editor that doesn't access
FSFS or caches — because everything happens locally and all operations that
use the FS layer have a chance of using the cache.
* subversion/libsvn_ra_local/ra_plugin.c
(make_reporter): Pass 0 as zero_copy_limit when creating the update
reporter.
Modified:
subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c
Modified: subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c?rev=1718167&r1=1718166&r2=1718167&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c (original)
+++ subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c Sun Dec 6 12:02:00 2015
@@ -360,8 +360,13 @@ make_reporter(svn_ra_session_t *session,
edit_baton,
NULL,
NULL,
- 1024 * 1024, /* process-local transfers
- should be fast */
+ 0, /* Disable zero-copy codepath, because
+ RA API users are unaware about the
+ zero-copy code path limitation (do
+ not access FSFS data structures
+ and, hence, caches). See notes
+ to svn_repos_begin_report3() for
+ additional details. */
result_pool));
/* Wrap the report baton given us by the repos layer with our own
Re: svn commit: r1718167 -/subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Bert Huijben <be...@qqmail.nl> writes:
> Any ideas how this affects editor v2 (And how it is used in JavaHL for 1.9)?
> Could this hit the limitations?
Ev2 in JavaHL is not affected by this problem.
When JavaHL asks the RA layer to drive the editor, it passes an Ev2-to-Ev1
shim as the editor. Within this shim, applying delta is handled by the
ev2_apply_textdelta() function, and, due to how it's written, this happens
without any function calls that could access the cache. Hence, it cannot
hit the limitations of the zero-copy code path.
Regards,
Evgeny Kotkov
RE: svn commit: r1718167 -/subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Evgeny Kotkov [mailto:evgeny.kotkov@visualsvn.com]
> Sent: maandag 7 december 2015 12:11
> To: Bert Huijben <be...@qqmail.nl>
> Cc: Subversion Development <de...@subversion.apache.org>
> Subject: Re: svn commit: r1718167 -
> /subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c
>
> Bert Huijben <be...@qqmail.nl> writes:
>
> > What is the real problem you try to solve here?
> >
> > Ra apis are not able to support other apis during callbacks or when
> > editors are open… This is part of the ra contract as documented in svn_ra.h.
> >
> > I don’t think we explicitly want to support users who break this contract.
>
> [...]
>
> > Ra local supports a lot of things because it is a very thin layer over
> > the lower layers… (I once accidentally found out that you can get very
> > far passing bad batons during commit). But the only thing we promise
> > is that it supports the ra contract.
>
> This change is not about providing support for cases when someone violates
> the RA contract:
>
> * The caller may not perform any RA operations using @a session before
> * finishing the report, and may not perform any RA operations using
> * @a session from within the editing operations of @a update_editor.
>
> That's because the situation is different when zero-copy code path is being
> used. In that case, the caller can't use the FS API or any other API functions
> using FS API from the callbacks, even if they occur on a completely unrelated
> object. This is so because the zero-copy send happens while holding the lock
> for the cache. The cache object is a per-process singleton, so, the callback is
> executed while basically holding a global mutex.
>
> We are lucky that, for instance, svnsync doesn't use the reporter in its
> implementation, but an arbitrary svnsync-like application or other API users
> have a chance of hitting the described problems once they perform an API
> call from within the editor callback. As another example, our diff editor can
> perform RA calls using an extra RA session in some cases. If a call like that
> happened from within a zero-copy code path, it would cause problems in the
> ra_local case.
Thanks for this explanation. This wasn't clear for me when reading the original patch and the backport proposal. With that explanation I would have added my +1.
Any ideas how this affects editor v2 (And how it is used in JavaHL for 1.9)? Could this hit the limitations?
Bert
Re: svn commit: r1718167 -/subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Bert Huijben <be...@qqmail.nl> writes:
> What is the real problem you try to solve here?
>
> Ra apis are not able to support other apis during callbacks or when editors
> are open… This is part of the ra contract as documented in svn_ra.h.
>
> I don’t think we explicitly want to support users who break this contract.
[...]
> Ra local supports a lot of things because it is a very thin layer over the
> lower layers… (I once accidentally found out that you can get very far
> passing bad batons during commit). But the only thing we promise is that it
> supports the ra contract.
This change is not about providing support for cases when someone violates
the RA contract:
* The caller may not perform any RA operations using @a session before
* finishing the report, and may not perform any RA operations using
* @a session from within the editing operations of @a update_editor.
That's because the situation is different when zero-copy code path is being
used. In that case, the caller can't use the FS API or any other API functions
using FS API from the callbacks, even if they occur on a completely unrelated
object. This is so because the zero-copy send happens while holding the lock
for the cache. The cache object is a per-process singleton, so, the callback
is executed while basically holding a global mutex.
We are lucky that, for instance, svnsync doesn't use the reporter in its
implementation, but an arbitrary svnsync-like application or other API users
have a chance of hitting the described problems once they perform an API
call from within the editor callback. As another example, our diff editor
can perform RA calls using an extra RA session in some cases. If a call like
that happened from within a zero-copy code path, it would cause problems in
the ra_local case.
Regards,
Evgeny Kotkov
RE: svn commit: r1718167
-/subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c
Posted by Bert Huijben <be...@qqmail.nl>.
What is the real problem you try to solve here?
Ra apis are not able to support other apis during callbacks or when editors are open… This is part of the ra contract as documented in svn_ra.h.
I don’t think we explicitly want to support users who break this contract.
That some things may work, doesn’t make them supported… which brings me back to the original question.
I’m unable to review changes like this… Without further explanation I don’t see any reason for this change… and certainly no reason for directly backporting this.
There are hundreds of apis that segfault on passing NULL, while some others may check for them as side effect of how they are implemented. That doesn’t necessarily make that part of their api contract.
That an api may be abused doesn’t require us to support abusing it.
Ra local supports a lot of things because it is a very thin layer over the lower layers… (I once accidentally found out that you can get very far passing bad batons during commit). But the only thing we promise is that it supports the ra contract.
Bert
Sent from Outlook Mail for Windows 10 phone
From: kotkov@apache.org
Sent: zondag 6 december 2015 13:02
To: commits@subversion.apache.org
Subject: svn commit: r1718167 -/subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c
Author: kotkov
Date: Sun Dec 6 12:02:00 2015
New Revision: 1718167
URL: http://svn.apache.org/viewvc?rev=1718167&view=rev
Log:
Disable zero-copy code path for ra_local update reporters.
The known and documented limitation of svn_repos_begin_report3() is that
with zero-copy enabled, the delta editor callbacks cannot access FSFS
or use Subversion caches directly. This limitation comes from the fact
that sending delta using the zero-copy code path happens from within a cache
access wrapper — that is, while holding the lock. If a particular delta
consumer happens to access or invalidate the cache, bad things could happen,
spanning from UB due to accessing a dangling pointer to a deadlock caused by
an attempt to take a non-recursive (blocking) lock, that has already been
taken by the same thread.
Within ra_local, we cannot be sure that arbitrary callers of our public
API, namely, svn_ra_do_update3(), svn_ra_do_switch3() or svn_ra_do_status2(),
are aware of this limitation and pass-in the delta editor that doesn't access
FSFS or caches — because everything happens locally and all operations that
use the FS layer have a chance of using the cache.
* subversion/libsvn_ra_local/ra_plugin.c
(make_reporter): Pass 0 as zero_copy_limit when creating the update
reporter.
Modified:
subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c
Modified: subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c?rev=1718167&r1=1718166&r2=1718167&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c (original)
+++ subversion/trunk/subversion/libsvn_ra_local/ra_plugin.c Sun Dec 6 12:02:00 2015
@@ -360,8 +360,13 @@ make_reporter(svn_ra_session_t *session,
edit_baton,
NULL,
NULL,
- 1024 * 1024, /* process-local transfers
- should be fast */
+ 0, /* Disable zero-copy codepath, because
+ RA API users are unaware about the
+ zero-copy code path limitation (do
+ not access FSFS data structures
+ and, hence, caches). See notes
+ to svn_repos_begin_report3() for
+ additional details. */
result_pool));
/* Wrap the report baton given us by the repos layer with our own