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.mar...@wandisco.com> wrote: >>>> Ivan Zhakov <i...@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
