You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@wandisco.com> on 2015/03/04 09:27:47 UTC
Thread-safe error location in maintainer mode
There was some discussion on #svn-dev recently about making the error
location tracing (see libsvn_subr/error.c and svn_error__locate)
thread-safe, by using platform- and compiler-specific flags for making
the location variables thread-safe.
It turns out that, since we now require APR 1.3+ which provides
unmanaged pools, we can now safely do this by using APR's threadkey API.
See:
https://paste.apache.org/fG82
With this patch, all tests pass for on my Mac in parallel mode and the
error stacks look sane. But, before committing the change, I'd like
someone else to review the patch because it's just a wee bit tricky in
places what with all the #ifdefs.
-- Brane
Re: Thread-safe error location in maintainer mode
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@wandisco.com> writes:
> I'll fix the comment ... but a typedef for the svn_atomic__init_once
> handler wouldn't really help, since you can't use that to define a function.
I would use it in the comment:
Implements svn_atomic__init_once_callback_t.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Thread-safe error location in maintainer mode
Posted by Branko Čibej <br...@wandisco.com>.
On 04.03.2015 13:23, Philip Martin wrote:
> Branko Čibej <br...@wandisco.com> writes:
>
>> error stacks look sane. But, before committing the change, I'd like
>> someone else to review the patch because it's just a wee bit tricky in
>> places what with all the #ifdefs.
> Looks OK, a few minor things:
>
>> Index: subversion/libsvn_subr/error.c
>> ===================================================================
>> --- subversion/libsvn_subr/error.c (revision 1663941)
>> +++ subversion/libsvn_subr/error.c (working copy)
>> @@ -28,6 +28,10 @@
>> #include <apr_pools.h>
>> #include <apr_strings.h>
>>
>> +#if defined(SVN_DEBUG) && APR_HAS_THREADS
>> +#include <apr_thread_proc.h>
>> +#endif
> Why not an unconditional include?
I prefer to keep it conditional so that bugs in fiddling with the
#ifdefs lower down will cause a compiler error.
>> -/* XXX TODO: Define mutex here #if APR_HAS_THREADS */
>> +/* No-op destructor for apr_threadkey_private_create(). */
>> +static void null_threadkey_dtor(void *stuff) {}
>> +
>> +/* Init-once function for the thread-local keys.
>> + This function will never return an error. */
> It is easier for the reader if "svn_atomic__init_once" is explict rather
> than implied by "Init-once". If svn_atomic__init_once used a typedef
> for the callback that would be even better, but that is a separate
> change.
I'll fix the comment ... but a typedef for the svn_atomic__init_once
handler wouldn't really help, since you can't use that to define a function.
-- Brane
Re: Thread-safe error location in maintainer mode
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@wandisco.com> writes:
> error stacks look sane. But, before committing the change, I'd like
> someone else to review the patch because it's just a wee bit tricky in
> places what with all the #ifdefs.
Looks OK, a few minor things:
> Index: subversion/libsvn_subr/error.c
> ===================================================================
> --- subversion/libsvn_subr/error.c (revision 1663941)
> +++ subversion/libsvn_subr/error.c (working copy)
> @@ -28,6 +28,10 @@
> #include <apr_pools.h>
> #include <apr_strings.h>
>
> +#if defined(SVN_DEBUG) && APR_HAS_THREADS
> +#include <apr_thread_proc.h>
> +#endif
Why not an unconditional include?
> +
> #include <zlib.h>
>
> #ifndef SVN_ERR__TRACING
> @@ -38,12 +42,55 @@
> #include "svn_pools.h"
> #include "svn_utf.h"
>
> +#include "private/svn_error_private.h"
> +#include "svn_private_config.h"
> +
> +#if defined(SVN_DEBUG) && APR_HAS_THREADS
> +#include "private/svn_atomic.h"
> +#include "pools.h"
> +#endif
Again, why not unconditional?
> +
> +
> #ifdef SVN_DEBUG
> -/* XXX FIXME: These should be protected by a thread mutex.
> - svn_error__locate and make_error_internal should cooperate
> - in locking and unlocking it. */
> +# if APR_HAS_THREADS
> +static apr_threadkey_t *error_file_key = NULL;
> +static apr_threadkey_t *error_line_key = NULL;
>
> -/* XXX TODO: Define mutex here #if APR_HAS_THREADS */
> +/* No-op destructor for apr_threadkey_private_create(). */
> +static void null_threadkey_dtor(void *stuff) {}
> +
> +/* Init-once function for the thread-local keys.
> + This function will never return an error. */
It is easier for the reader if "svn_atomic__init_once" is explict rather
than implied by "Init-once". If svn_atomic__init_once used a typedef
for the callback that would be even better, but that is a separate
change.
> +static svn_error_t *
> +locate_init_once(void *ignored_baton, apr_pool_t *ignored_pool)
> +{
> + /* Strictly speaking, this is a memory leak, since we're creating an
> + unmanaged, top-level pool and never destroying it. We do this
> + because this pool controls the lifetime of the thread-local
> + storage for error locations, and that storage must always be
> + available. */
> + apr_pool_t *threadkey_pool = svn_pool__create_unmanaged(TRUE);
> + apr_status_t status;
> +
> + status = apr_threadkey_private_create(&error_file_key,
> + null_threadkey_dtor,
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Thread-safe error location in maintainer mode
Posted by Branko Čibej <br...@wandisco.com>.
On 04.03.2015 14:31, Ivan Zhakov wrote:
> On 4 March 2015 at 16:07, Branko Čibej <br...@wandisco.com> wrote:
>> On 04.03.2015 13:50, Ivan Zhakov wrote:
>>> On 4 March 2015 at 11:27, Branko Čibej <br...@wandisco.com> wrote:
>>>> There was some discussion on #svn-dev recently about making the error
>>>> location tracing (see libsvn_subr/error.c and svn_error__locate)
>>>> thread-safe, by using platform- and compiler-specific flags for making
>>>> the location variables thread-safe.
>>>>
>>>> It turns out that, since we now require APR 1.3+ which provides
>>>> unmanaged pools, we can now safely do this by using APR's threadkey API.
>>>> See:
>>>>
>>>> https://paste.apache.org/fG82
>>>>
>>>> With this patch, all tests pass for on my Mac in parallel mode and the
>>>> error stacks look sane. But, before committing the change, I'd like
>>>> someone else to review the patch because it's just a wee bit tricky in
>>>> places what with all the #ifdefs.
>>>>
>>> I was thinking about similar change, but as far I remember it's not
>>> safe to use svn_atomic__init_once() in error.c, because
>>> svn_atomic__init_once() uses svn_error_*() API. This may lead circular
>>> initialization of svn_error_*() tracing infrastructure.
>> Nope. svn_atomic__init_once only begins using the svn_error_* functions
>> after the init function has returned, at which point the tracing logic
>> is initialized. Furthermore, I intentionally implemented this particular
>> init function so that it never returns an error.
>>
>> Although, looking at the implementation of svn_atomic__init_once, we
>> really shouldn't be optimizing the error case with all the #if
>> APR_HAS_THREADS blocks.
>>
> It may be doesn't cause problems now, but svn_atomic__init_once()
> implementation may change in future. So I really prefer to
> re-implement svn_atomic__init_once() in error.c. It should not be big
> problem IMO, but could avoid some weird deadlocks in future.
I don't believe it's worth the trouble, and even less worth the code
duplication. Consider the following:
* If svn_atomic__init_once does anything but spin in other threads
while the init function is running, it's broken.
* Before the init function is called, and also after it completes, the
error location bits are guaranteed to be in a valid state.
So the only way to break anything would be if svn_atomic__init_once is
broken, either due to doing things it shouldn't or allowing some other
thread to pass through while the init-func is running.
It's a definite waste of effort to duplicate code and maintain it
forever on the off chance that someone is going to "optimize"
svn_atomic__init_once so that doesn't follow its declared contract. Lots
of other things will break if that happens.
-- Brane
Re: Thread-safe error location in maintainer mode
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 4 March 2015 at 16:07, Branko Čibej <br...@wandisco.com> wrote:
> On 04.03.2015 13:50, Ivan Zhakov wrote:
>> On 4 March 2015 at 11:27, Branko Čibej <br...@wandisco.com> wrote:
>>> There was some discussion on #svn-dev recently about making the error
>>> location tracing (see libsvn_subr/error.c and svn_error__locate)
>>> thread-safe, by using platform- and compiler-specific flags for making
>>> the location variables thread-safe.
>>>
>>> It turns out that, since we now require APR 1.3+ which provides
>>> unmanaged pools, we can now safely do this by using APR's threadkey API.
>>> See:
>>>
>>> https://paste.apache.org/fG82
>>>
>>> With this patch, all tests pass for on my Mac in parallel mode and the
>>> error stacks look sane. But, before committing the change, I'd like
>>> someone else to review the patch because it's just a wee bit tricky in
>>> places what with all the #ifdefs.
>>>
>> I was thinking about similar change, but as far I remember it's not
>> safe to use svn_atomic__init_once() in error.c, because
>> svn_atomic__init_once() uses svn_error_*() API. This may lead circular
>> initialization of svn_error_*() tracing infrastructure.
>
> Nope. svn_atomic__init_once only begins using the svn_error_* functions
> after the init function has returned, at which point the tracing logic
> is initialized. Furthermore, I intentionally implemented this particular
> init function so that it never returns an error.
>
> Although, looking at the implementation of svn_atomic__init_once, we
> really shouldn't be optimizing the error case with all the #if
> APR_HAS_THREADS blocks.
>
It may be doesn't cause problems now, but svn_atomic__init_once()
implementation may change in future. So I really prefer to
re-implement svn_atomic__init_once() in error.c. It should not be big
problem IMO, but could avoid some weird deadlocks in future.
--
Ivan Zhakov
Re: Thread-safe error location in maintainer mode
Posted by Branko Čibej <br...@wandisco.com>.
On 04.03.2015 13:50, Ivan Zhakov wrote:
> On 4 March 2015 at 11:27, Branko Čibej <br...@wandisco.com> wrote:
>> There was some discussion on #svn-dev recently about making the error
>> location tracing (see libsvn_subr/error.c and svn_error__locate)
>> thread-safe, by using platform- and compiler-specific flags for making
>> the location variables thread-safe.
>>
>> It turns out that, since we now require APR 1.3+ which provides
>> unmanaged pools, we can now safely do this by using APR's threadkey API.
>> See:
>>
>> https://paste.apache.org/fG82
>>
>> With this patch, all tests pass for on my Mac in parallel mode and the
>> error stacks look sane. But, before committing the change, I'd like
>> someone else to review the patch because it's just a wee bit tricky in
>> places what with all the #ifdefs.
>>
> I was thinking about similar change, but as far I remember it's not
> safe to use svn_atomic__init_once() in error.c, because
> svn_atomic__init_once() uses svn_error_*() API. This may lead circular
> initialization of svn_error_*() tracing infrastructure.
Nope. svn_atomic__init_once only begins using the svn_error_* functions
after the init function has returned, at which point the tracing logic
is initialized. Furthermore, I intentionally implemented this particular
init function so that it never returns an error.
Although, looking at the implementation of svn_atomic__init_once, we
really shouldn't be optimizing the error case with all the #if
APR_HAS_THREADS blocks.
-- Brane
Re: Thread-safe error location in maintainer mode
Posted by Branko Čibej <br...@wandisco.com>.
On 04.03.2015 17:04, Philip Martin wrote:
> Branko Čibej <br...@wandisco.com> writes:
>
>> And here it is: the whole atomic.c file
>>
>> https://paste.apache.org/2iyN
>>
>> and the diff against current trunk:
>>
>> https://paste.apache.org/cK1R
>>
> I prefer patches to be included in the email. Ideally inline, as far as
> I am concerned, or as an attachment.
In this case, looking at the whole file is actually easier to review ...
anyway, here's the latest version of the patch:
Index: subversion/libsvn_subr/atomic.c
===================================================================
--- subversion/libsvn_subr/atomic.c (revision 1663988)
+++ subversion/libsvn_subr/atomic.c (working copy)
@@ -42,44 +42,53 @@
/* We have to call init_func exactly once. Because APR
doesn't have statically-initialized mutexes, we implement a poor
man's spinlock using svn_atomic_cas. */
+
+ svn_error_t *err = SVN_NO_ERROR;
svn_atomic_t status = svn_atomic_cas(global_status,
SVN_ATOMIC_START_INIT,
SVN_ATOMIC_UNINITIALIZED);
- if (status == SVN_ATOMIC_UNINITIALIZED)
+ for (;;)
{
- svn_error_t *err = init_func(baton, pool);
- if (err)
+ switch (status)
{
-#if APR_HAS_THREADS
- /* Tell other threads that the initialization failed. */
- svn_atomic_cas(global_status,
- SVN_ATOMIC_INIT_FAILED,
- SVN_ATOMIC_START_INIT);
-#endif
+ case SVN_ATOMIC_UNINITIALIZED:
+ err = init_func(baton, pool);
+ if (err)
+ {
+ status = svn_atomic_cas(global_status,
+ SVN_ATOMIC_INIT_FAILED,
+ SVN_ATOMIC_START_INIT);
+ }
+ else
+ {
+ status = svn_atomic_cas(global_status,
+ SVN_ATOMIC_INITIALIZED,
+ SVN_ATOMIC_START_INIT);
+ }
+
+ /* Take another spin through the switch to make sure that we
+ have just set a valid value. */
+ continue;
+
+ case SVN_ATOMIC_START_INIT:
+ /* Wait for the init function to complete. */
+ apr_sleep(APR_USEC_PER_SEC / 1000);
+ status = svn_atomic_cas(global_status,
+ SVN_ATOMIC_UNINITIALIZED,
+ SVN_ATOMIC_UNINITIALIZED);
+ continue;
+
+ case SVN_ATOMIC_INIT_FAILED:
return svn_error_create(SVN_ERR_ATOMIC_INIT_FAILURE, err,
"Couldn't perform atomic initialization");
+
+ case SVN_ATOMIC_INITIALIZED:
+ return SVN_NO_ERROR;
+
+ default:
+ /* Something went seriously wrong with the atomic operations. */
+ abort();
}
- svn_atomic_cas(global_status,
- SVN_ATOMIC_INITIALIZED,
- SVN_ATOMIC_START_INIT);
}
-#if APR_HAS_THREADS
- /* Wait for whichever thread is performing initialization to finish. */
- /* XXX FIXME: Should we have a maximum wait here, like we have in
- the Windows file IO spinner? */
- else while (status != SVN_ATOMIC_INITIALIZED)
- {
- if (status == SVN_ATOMIC_INIT_FAILED)
- return svn_error_create(SVN_ERR_ATOMIC_INIT_FAILURE, NULL,
- "Couldn't perform atomic initialization");
-
- apr_sleep(APR_USEC_PER_SEC / 1000);
- status = svn_atomic_cas(global_status,
- SVN_ATOMIC_UNINITIALIZED,
- SVN_ATOMIC_UNINITIALIZED);
- }
-#endif /* APR_HAS_THREADS */
-
- return SVN_NO_ERROR;
}
Re: Thread-safe error location in maintainer mode
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@wandisco.com> writes:
> And here it is: the whole atomic.c file
>
> https://paste.apache.org/2iyN
>
> and the diff against current trunk:
>
> https://paste.apache.org/cK1R
>
I prefer patches to be included in the email. Ideally inline, as far as
I am concerned, or as an attachment.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Thread-safe error location in maintainer mode
Posted by Branko Čibej <br...@wandisco.com>.
On 04.03.2015 16:21, Branko Čibej wrote:
> On 04.03.2015 15:26, Branko Čibej wrote:
>> On 04.03.2015 14:56, Ivan Zhakov wrote:
>>> On 4 March 2015 at 16:48, Philip Martin <ph...@wandisco.com> wrote:
>>>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>>>
>>>>> I was thinking about similar change, but as far I remember it's not
>>>>> safe to use svn_atomic__init_once() in error.c, because
>>>>> svn_atomic__init_once() uses svn_error_*() API. This may lead circular
>>>>> initialization of svn_error_*() tracing infrastructure.
>>>> That does need to be considered.
>>>>
>>>> The atomic init callback in this case is written to never return an
>>>> error so the only errors to consider are those raised by
>>>> svn_atomic__init_once itself. Any such error would be a failure of
>>>> svn_atomic_cas. Possible causes: the svn_atomic_t passed in is something
>>>> other than 0/1/2 or the svn_atomic_t was read-only. Anything else?
>>>>
>>>> If I initialize:
>>>>
>>>> static volatile svn_atomic_t init_status = 10;
>>>>
>>>> the client does go into an infinite loop when attempting to raise an
>>>> error but that doesn't have anything to do with this patch, the same
>>>> happens with any call to svn_atomic__init_once.
>>>>
>>>> If I initialize
>>>>
>>>> static volatile svn_atomic_t init_status = 2;
>>>>
>>> This is exactly what I tried two minutes ago.
>> Well, if we get into such a state, there's really not much we can do,
>> regardless of any use of svn_error_* functions.
>>
>>> I agree that currently it's not possible to get stackoverflow, but I
>>> prefer to avoid circular dependency anyway.
>>> svn_atomic__init_once_no_error() seems to nice solution for this
>>> _potential_ problem.
>> That wouldn't solve the memory-overwrite cases, nor the infinite loops.
>> It would just generate a false warm fuzzy feeling that you can't recurse
>> ... which is, strictly speaking, not true, because recursion depends on
>> the specific implementation of the init function.
>>
>> svn_error__init_once could be made more bullet-proof by checking the
>> actual value against the expected set of values and aborting if it's
>> incorrect, instead of raising an error. That's the safest way to handle
>> this sort of thing. We should also remove the #if APR_HAS_THREADS blocks
>> in the implementation, because the control variable should be set to 2
>> (SVN_ATOMIC_INIT_FAILED) regardless of threading support.
> And here it is: the whole atomic.c file
>
> https://paste.apache.org/2iyN
>
> and the diff against current trunk:
>
> https://paste.apache.org/cK1R
>
> I think this implementation is a lot clearer and also safer than the
> previous one. Note that the error is created exactly once in the code. I
> believe this implementation avoids the possible infinite recursion.
(BTW, I noticed that I removed the apr_sleep; it's now reinstated in the
right place in my working copy.)
-- Brane
Re: Thread-safe error location in maintainer mode
Posted by Branko Čibej <br...@wandisco.com>.
On 04.03.2015 15:26, Branko Čibej wrote:
> On 04.03.2015 14:56, Ivan Zhakov wrote:
>> On 4 March 2015 at 16:48, Philip Martin <ph...@wandisco.com> wrote:
>>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>>
>>>> I was thinking about similar change, but as far I remember it's not
>>>> safe to use svn_atomic__init_once() in error.c, because
>>>> svn_atomic__init_once() uses svn_error_*() API. This may lead circular
>>>> initialization of svn_error_*() tracing infrastructure.
>>> That does need to be considered.
>>>
>>> The atomic init callback in this case is written to never return an
>>> error so the only errors to consider are those raised by
>>> svn_atomic__init_once itself. Any such error would be a failure of
>>> svn_atomic_cas. Possible causes: the svn_atomic_t passed in is something
>>> other than 0/1/2 or the svn_atomic_t was read-only. Anything else?
>>>
>>> If I initialize:
>>>
>>> static volatile svn_atomic_t init_status = 10;
>>>
>>> the client does go into an infinite loop when attempting to raise an
>>> error but that doesn't have anything to do with this patch, the same
>>> happens with any call to svn_atomic__init_once.
>>>
>>> If I initialize
>>>
>>> static volatile svn_atomic_t init_status = 2;
>>>
>> This is exactly what I tried two minutes ago.
> Well, if we get into such a state, there's really not much we can do,
> regardless of any use of svn_error_* functions.
>
>> I agree that currently it's not possible to get stackoverflow, but I
>> prefer to avoid circular dependency anyway.
>> svn_atomic__init_once_no_error() seems to nice solution for this
>> _potential_ problem.
> That wouldn't solve the memory-overwrite cases, nor the infinite loops.
> It would just generate a false warm fuzzy feeling that you can't recurse
> ... which is, strictly speaking, not true, because recursion depends on
> the specific implementation of the init function.
>
> svn_error__init_once could be made more bullet-proof by checking the
> actual value against the expected set of values and aborting if it's
> incorrect, instead of raising an error. That's the safest way to handle
> this sort of thing. We should also remove the #if APR_HAS_THREADS blocks
> in the implementation, because the control variable should be set to 2
> (SVN_ATOMIC_INIT_FAILED) regardless of threading support.
And here it is: the whole atomic.c file
https://paste.apache.org/2iyN
and the diff against current trunk:
https://paste.apache.org/cK1R
I think this implementation is a lot clearer and also safer than the
previous one. Note that the error is created exactly once in the code. I
believe this implementation avoids the possible infinite recursion.
-- Brane
Re: Thread-safe error location in maintainer mode
Posted by Branko Čibej <br...@wandisco.com>.
On 04.03.2015 14:56, Ivan Zhakov wrote:
> On 4 March 2015 at 16:48, Philip Martin <ph...@wandisco.com> wrote:
>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>
>>> I was thinking about similar change, but as far I remember it's not
>>> safe to use svn_atomic__init_once() in error.c, because
>>> svn_atomic__init_once() uses svn_error_*() API. This may lead circular
>>> initialization of svn_error_*() tracing infrastructure.
>> That does need to be considered.
>>
>> The atomic init callback in this case is written to never return an
>> error so the only errors to consider are those raised by
>> svn_atomic__init_once itself. Any such error would be a failure of
>> svn_atomic_cas. Possible causes: the svn_atomic_t passed in is something
>> other than 0/1/2 or the svn_atomic_t was read-only. Anything else?
>>
>> If I initialize:
>>
>> static volatile svn_atomic_t init_status = 10;
>>
>> the client does go into an infinite loop when attempting to raise an
>> error but that doesn't have anything to do with this patch, the same
>> happens with any call to svn_atomic__init_once.
>>
>> If I initialize
>>
>> static volatile svn_atomic_t init_status = 2;
>>
> This is exactly what I tried two minutes ago.
Well, if we get into such a state, there's really not much we can do,
regardless of any use of svn_error_* functions.
> I agree that currently it's not possible to get stackoverflow, but I
> prefer to avoid circular dependency anyway.
> svn_atomic__init_once_no_error() seems to nice solution for this
> _potential_ problem.
That wouldn't solve the memory-overwrite cases, nor the infinite loops.
It would just generate a false warm fuzzy feeling that you can't recurse
... which is, strictly speaking, not true, because recursion depends on
the specific implementation of the init function.
svn_error__init_once could be made more bullet-proof by checking the
actual value against the expected set of values and aborting if it's
incorrect, instead of raising an error. That's the safest way to handle
this sort of thing. We should also remove the #if APR_HAS_THREADS blocks
in the implementation, because the control variable should be set to 2
(SVN_ATOMIC_INIT_FAILED) regardless of threading support.
-- Brane
Re: Thread-safe error location in maintainer mode
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 4 March 2015 at 16:48, Philip Martin <ph...@wandisco.com> wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
>> I was thinking about similar change, but as far I remember it's not
>> safe to use svn_atomic__init_once() in error.c, because
>> svn_atomic__init_once() uses svn_error_*() API. This may lead circular
>> initialization of svn_error_*() tracing infrastructure.
>
> That does need to be considered.
>
> The atomic init callback in this case is written to never return an
> error so the only errors to consider are those raised by
> svn_atomic__init_once itself. Any such error would be a failure of
> svn_atomic_cas. Possible causes: the svn_atomic_t passed in is something
> other than 0/1/2 or the svn_atomic_t was read-only. Anything else?
>
> If I initialize:
>
> static volatile svn_atomic_t init_status = 10;
>
> the client does go into an infinite loop when attempting to raise an
> error but that doesn't have anything to do with this patch, the same
> happens with any call to svn_atomic__init_once.
>
> If I initialize
>
> static volatile svn_atomic_t init_status = 2;
>
This is exactly what I tried two minutes ago.
> the client recurses until it SEGVs when svn_atomic__init_once tries to
> raise an error, that is caused by this patch. The error that
> svn_atomic__init_once is trying to raise would be discarded by
> svn_error__locate.
>
> A memory overwrite bug could cause the svn_atomic_t used by
> svn_error__locate to become 2 but equally a memory overwrite bug could
> set any other svn_atomic_t to 10 and cause an infinite loop. We could
> add a "no errors" flag to svn_atomic__init_once to avoid the
> recursion/SEGV caused when the svn_atomic_t is 2 but trying to handle
> memory overwrite is never going to be reliable.
>
> Overall I don't think the circular problem is a problem in practice.
>
I agree that currently it's not possible to get stackoverflow, but I
prefer to avoid circular dependency anyway.
svn_atomic__init_once_no_error() seems to nice solution for this
_potential_ problem.
--
Ivan Zhakov
Re: Thread-safe error location in maintainer mode
Posted by Philip Martin <ph...@wandisco.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> I was thinking about similar change, but as far I remember it's not
> safe to use svn_atomic__init_once() in error.c, because
> svn_atomic__init_once() uses svn_error_*() API. This may lead circular
> initialization of svn_error_*() tracing infrastructure.
That does need to be considered.
The atomic init callback in this case is written to never return an
error so the only errors to consider are those raised by
svn_atomic__init_once itself. Any such error would be a failure of
svn_atomic_cas. Possible causes: the svn_atomic_t passed in is something
other than 0/1/2 or the svn_atomic_t was read-only. Anything else?
If I initialize:
static volatile svn_atomic_t init_status = 10;
the client does go into an infinite loop when attempting to raise an
error but that doesn't have anything to do with this patch, the same
happens with any call to svn_atomic__init_once.
If I initialize
static volatile svn_atomic_t init_status = 2;
the client recurses until it SEGVs when svn_atomic__init_once tries to
raise an error, that is caused by this patch. The error that
svn_atomic__init_once is trying to raise would be discarded by
svn_error__locate.
A memory overwrite bug could cause the svn_atomic_t used by
svn_error__locate to become 2 but equally a memory overwrite bug could
set any other svn_atomic_t to 10 and cause an infinite loop. We could
add a "no errors" flag to svn_atomic__init_once to avoid the
recursion/SEGV caused when the svn_atomic_t is 2 but trying to handle
memory overwrite is never going to be reliable.
Overall I don't think the circular problem is a problem in practice.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Thread-safe error location in maintainer mode
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 4 March 2015 at 11:27, Branko Čibej <br...@wandisco.com> wrote:
> There was some discussion on #svn-dev recently about making the error
> location tracing (see libsvn_subr/error.c and svn_error__locate)
> thread-safe, by using platform- and compiler-specific flags for making
> the location variables thread-safe.
>
> It turns out that, since we now require APR 1.3+ which provides
> unmanaged pools, we can now safely do this by using APR's threadkey API.
> See:
>
> https://paste.apache.org/fG82
>
> With this patch, all tests pass for on my Mac in parallel mode and the
> error stacks look sane. But, before committing the change, I'd like
> someone else to review the patch because it's just a wee bit tricky in
> places what with all the #ifdefs.
>
I was thinking about similar change, but as far I remember it's not
safe to use svn_atomic__init_once() in error.c, because
svn_atomic__init_once() uses svn_error_*() API. This may lead circular
initialization of svn_error_*() tracing infrastructure.
--
Ivan Zhakov
Re: Thread-safe error location in maintainer mode
Posted by Branko Čibej <br...@wandisco.com>.
On 04.03.2015 11:18, Philip Martin wrote:
> Branko Čibej <br...@wandisco.com> writes:
>
>> https://paste.apache.org/fG82
> When I download that patch on my Linux box it ends:
>
> Added: svn:eol-style^M
> ## -0,0 +1 ##^M
> +native^M
> \ No newline at end of property
>
> it has Windows line endings, which is fine, but it is missing a line on
> the final "No newline" line, which seems wrong. Where did that the
> newline stripped? Was it your svn client, the tool you used to upload
> the patch, or the paste site itself?
>
> I tried firefox's "Save as" and curl:
>
> curl 'https://paste.apache.org/fG82?action=download'
It's a copy-paset from Emacs, and no, I have no idea where the Windows
line endings come from; probably the pastebin itself?
I'll try uploading the patch instead into a new bin ...
https://paste.apache.org/eGkC
Apparently the Windows line endings are still there.
-- Brane
Re: Thread-safe error location in maintainer mode
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@wandisco.com> writes:
> https://paste.apache.org/fG82
When I download that patch on my Linux box it ends:
Added: svn:eol-style^M
## -0,0 +1 ##^M
+native^M
\ No newline at end of property
it has Windows line endings, which is fine, but it is missing a line on
the final "No newline" line, which seems wrong. Where did that the
newline stripped? Was it your svn client, the tool you used to upload
the patch, or the paste site itself?
I tried firefox's "Save as" and curl:
curl 'https://paste.apache.org/fG82?action=download'
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*