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 <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.
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
Received on 2015-03-04 16:23:18 CET