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 <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.
(BTW, I noticed that I removed the apr_sleep; it's now reinstated in the
right place in my working copy.)
-- Brane
Received on 2015-03-04 16:26:42 CET