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.
> 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
