[Swig-devel] [ swig-Bugs-2813836 ] python/std_map.i fails to lock correctly

[SourceForge.net <no...@so...>]

Bugs item #2813836, was opened at 2009-06-29 18:51
Message generated for change (Comment added) made by tbble
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=101645&aid=2813836&group_id=1645

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: python
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Paul Hampson (tbble)
Assigned to: Nobody/Anonymous (nobody)
Summary: python/std_map.i fails to lock correctly

Initial Comment:
python/std_map.i does not have correct locking. In several functions, it takes the lock only for a PyErr_SetString and fails to protect a PyList_New or PyDict_New and related calls later on. In struct traits_asptr<std::map<K,T> >::asptr, no lock is taken at all.

I've attached a patch (against 1.3.38, but SVN indicates that this is the most recent version of this file) against Lib/python/std_map.i

----------------------------------------------------------------------

>Comment By: Paul Hampson (tbble)
Date: 2009-06-30 18:55

Message:
I don't have a testcase I can post at this point, but the issue is that at
least in the code I'm generating here, the only caller of the wrapper
functions generated by this file is calling the
SWIG_PYTHON_THREAD_BEGIN_ALLOW macro before calling them, which releases
the GIL. It does this for a whole bunch of the methods, most of which it's
fine for, because it then calls C++ functions between the ALLOWs.

To block out an example from my current _wrap.cxx code, the code which is
wrapping the values() method produced by the std_map.i looks like this:
SWIGINTERN PyObject *_wrap_MyMap_values(PyObject *SWIGUNUSEDPARM(self),
PyObject *args) {
 ...
 SWIG_PYTHON_THREAD_BEGIN_BLOCK
 // Parse args using Py* methods
 arg1 = reinterpret_cast< MyMap >* )(argp1);
 {
  SWIG_PYTHON_THREAD_BEGIN_ALLOW
  result = (PyObject *)mangled_myMap_values(arg1);
  SWIG_PYTHON_THREAD_END_ALLOW
 }
 ...
 SWIG_PYTHON_THREAD_END_BLOCK
 return
}

And this patch has definitely fixed random memory corruption which we
chased down to finding two threads running inside the python interpreter,
both apparently having gotten the same or at least overlapping blocks of
RAM back from PyMalloc. Using the python debugging interpreter, it fails
very quickly.

I dread to think how mod_python manages to break the documented reentrancy
behaviour of the GIL.

The other option is to remove the wrapping SWIG_PYTHON_THREAD_BEGIN_ALLOW,
although that's not provided by any of the .i files in my installation of
SWIG, so I don't think it's an option I can really apply locally.

----------------------------------------------------------------------

Comment By: Olly Betts (olly)
Date: 2009-06-30 15:04

Message:
Do you have a testcase which shows a problem with the current code?

For a non-director function wrapper, we should be called with the GIL
already locked, so there's no need to lock it, and trying to do so is
actually harmful under mod_python as it can cause deadlocks (due to bugs in
mod_python apparently).

So my initial feeling is that actually the locks for PyErr_SetString(),
etc are probably superfluous and should be removed rather than extended,
but I might be wrong.  A testcase which shows a problem would save a lot of
time, and also provide a regression test for this if it does fix a problem.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=101645&aid=2813836&group_id=1645