Re: svn_config_t is not thread-safe

On 03.09.2012 13:46, Joel Jirak wrote:
> When debugging the recent TSVN cert problem, I noticed that even
> though I specified a ssl cert in my servers file, I would sometimes
> get prompted to enter my certificate when repo-browsing. This would
> never happen when doing a show-log, but only when doing a repo-browse,
> and then only about half the time. Still, it happened frequently
> enough to make it reproducible.
>
> The repo browse functionality seems to be the most threaded of any
> TSVN operation. When I've trapped my problem in the debugger, here's
> a typical scenario, with the following three functions on the stack,
> in different stages of execution:
>
> TortoiseProc.exe!SVN::List()
>
> TortoiseProc.exe!SVNReadProperties::Refresh()
>
> TortoiseProc.exe!SVN::GetRepositoryRootAndUUID()
>
> The reason TSVN sometimes fails to read my servers file correctly is
> that SVNConfig is a singleton, but the svn struct svn_config_t
> (defined in config_impl.h) is not thread-safe for reading due to
> svn_config_t::tmp_key. tmp_key is used to hold the key from the
> config file you want (like a section name or an actual setting within
> a section) during a read. The read is interruptible, however, so,
> multiple readers stomp on each others' tmp_key values when
> simultaneously reading from a single instance of svn_config_t. Here's
> a patch with some trace statements that show the problem
> (svn_config_t_trace.patch). I've got a trace statement in
> svn_ra_neon__open, which will see an intermittent failure when calling
> svn_config_find_group().
>
> svn_ra_neon__open() calls
> svn_config_find_group(), which calls
> svn_config_enumerate2(), which calls
> find_option(), which calls
> apr_hash_get() with tmp_key. Unfortunately, this tmp_key can change
> while the hash lookup is going on.

Good catch!
Thanks a lot for the analysis.

> I would assume that svn_config_t is not thread-safe by design. Thus,
> TSVN should be changed. However, here's a hack patch that shows how
> to get rid up the shared state in tmp_key. This patch does fix my
> problem.

Actually, all svn APIs should be thread safe for reading.
APIs that are not thread safe are usually marked as such, for example
svn_utf_initialize():
"It is optional to call this function, but if it is used, no other svn
function may be in use in other threads during the call of this function
or when pool is cleared or destroyed. Initializing the UTF-8 routines
will improve performance."

So I really have to assume that this is a bug in the svn library.

I'll ask on the svn dev list for confirmation.

Stefan

-- 
        ___
   oo  // \\      "De Chelonian Mobile"
  (_,\/ \_/ \     TortoiseSVN
    \ \_/_\_/>    The coolest Interface to (Sub)Version Control
    /_/   \_\     http://tortoisesvn.net
------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3003237
To unsubscribe from this discussion, e-mail: [dev-unsubscribe_at_tortoisesvn.tigris.org].