On 04.03.2015 14:56, Ivan Zhakov wrote:
> On 4 March 2015 at 16:48, Philip Martin <philip.martin_at_wandisco.com> wrote:
>> Ivan Zhakov <ivan_at_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
Received on 2015-03-04 15:27:18 CET