Re: Thread-safe error location in maintainer mode

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;

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.

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*