Re: Non-thread-safe hash iterator [was: svn commit: r39768 - trunk/subversion/libsvn_fs_fs]

I (Julian Foad) wrote:
> Greg Stein wrote:
> > > +++ trunk/subversion/libsvn_fs_fs/fs_fs.c Fri Oct 2 10:22:06 2009 (r39768)
> > >...
> > > @@ -6185,18 +6172,17 @@ recover_find_max_ids(svn_fs_t *fs, svn_r
> > > iterpool = svn_pool_create(pool);
> > > for (hi = apr_hash_first(NULL, entries); hi; hi = apr_hash_next(hi))
>
> Hey, look - - - - - - - - - ^^^
>
> It's a case where we use the hash's non-thread-safe built-in iterator,
> isn't it? I don't know whether it's demonstrably harmful in this case
> but I think we want to avoid it (pass a pool instead of NULL).

A comment made by one of you (Brane? Greg?) on IRC the other day made me
think we want to avoid ever making use of apr_hash_t's built-in
iterator, as a matter of policy, in order to avoid potential
thread-safety problems, rather than analyzing each use of
apr_hash_this() to see if thread safety is an issue for that particular
instance. Can anyone confirm (or deny) that idea?

After applying the attached patch, the following instances would remain:

[[[
$ find subversion/ -name '*.[ch]' | xargs grep -n "apr_hash_first.*NULL"
subversion/libsvn_subr/hash.c:502: for (hi = apr_hash_first(NULL, hash); hi; hi = apr_hash_next(hi))
subversion/libsvn_client/merge.c:6700: for (hi = apr_hash_first(NULL, subtrees);
subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c:507: for (hi = apr_hash_first(NULL, hash); hi; hi = apr_hash_next(hi))
subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c:755: for (hi = apr_hash_first(NULL, hash); hi; hi = apr_hash_next(hi))
subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c:856: for (hi = apr_hash_first(NULL, hash); hi; hi = apr_hash_next(hi))
subversion/bindings/swig/ruby/libsvn_swig_ruby/swigutil_rb.c:1408: for (hi = apr_hash_first(NULL, hash); hi; hi = apr_hash_next(hi)) {
subversion/bindings/swig/perl/libsvn_swig_perl/swigutil_pl.c:240: for (hi = apr_hash_first(NULL, hash); hi; hi = apr_hash_next(hi)) {
]]]

The ones in merge.c and hash.c I can easily fix too by passing a pool to
those functions.

The ones in swigutil_*.c I'm not sure about, as they seem to be public
functions. Do they need this treatment?

- Julian

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2404913