You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2009/10/08 00:59:24 UTC
Non-thread-safe hash iterator [was: svn commit: r39768 -
trunk/subversion/libsvn_fs_fs]
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).
Not related to this commit, of course.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2404833
Re: Non-thread-safe hash iterator
Posted by David James <ja...@gmail.com>.
On Thu, Oct 8, 2009 at 7:16 AM, Julian Foad <ju...@btopenworld.com> wrote:
> I (Julian Foad) wrote:
>> 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.
>
> I applied the patch in r39869 and fixed the ones in merge.c and hash.c
> in r39873.
>
>> The ones in swigutil_*.c I'm not sure about, as they seem to be public
>> functions. Do they need this treatment?
>
> I don't know whether to touch those, not being familiar with SWIG.
Hi Julian,
I don't know what the threading policy of the Ruby or Perl bindings
are, but I can speak about the Python bindings.
The functions in swigutil_py.c aren't public functions, unless they
are explicitly exposed in a swig wrapper -- they generally are just
used internally by the SWIG bindings. The three functions you
mentioned (convert_hash, svn_swig_py_locationhash_to_dict, and
svn_swig_py_changed_path_hash_to_dict) are internal functions and are
not used directly by users of the bindings.
These three functions all assume that the the global interpreter lock
is being held prior to calling them, so I believe they do not need to
worry about threading. That said, if you want, you are welcome to
update them to use pools as a precautionary measure. (You probably
won't want to though, since you'll need to learn about SWIG to do
this.)
Cheers,
David
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2405308
Re: Non-thread-safe hash iterator
Posted by Julian Foad <ju...@btopenworld.com>.
I (Julian Foad) wrote:
> 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.
I applied the patch in r39869 and fixed the ones in merge.c and hash.c
in r39873.
> The ones in swigutil_*.c I'm not sure about, as they seem to be public
> functions. Do they need this treatment?
I don't know whether to touch those, not being familiar with SWIG.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2405025
Re: Non-thread-safe hash iterator [was: svn commit: r39768 -
trunk/subversion/libsvn_fs_fs]
Posted by Julian Foad <ju...@btopenworld.com>.
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