Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix shadow instance creation failure in Python 3 #137

Merged
merged 1 commit into from Feb 16, 2014

Conversation

hfalcic
Copy link
Contributor

@hfalcic hfalcic commented Feb 15, 2014

I managed to trace a very nasty Python interpreter segfault to an
allocation failure here. Adding this after the tp_new call:
if (PyErr_Occurred()) {
PyErr_Print();
}

results in output of 'TypeError: object() takes no parameters', followed
by a segfault that takes down the Python interpeter.

The 'object' constructor doesn't seem to be suitable for instantiating
SWIG shadow instances in this way, so simply use the constructor
function in the PyTypeObject 'tp_new' slot of data->newargs.

The 'if (inst)' check after this doesn't hurt in as much as it prevented
a segfault immediately after this failed allocation, but it doesn't help
much since the null pointer dereference will probably happen sooner or
later anyway.

I managed to trace a very nasty Python interpreter segfault to an
allocation failure here. Adding this after the tp_new call:
if (PyErr_Occurred()) {
    PyErr_Print();
}

results in output of 'TypeError: object() takes no parameters', followed
by a segfault that takes down the Python interpeter.

The 'object' constructor doesn't seem to be suitable for instantiating
SWIG shadow instances in this way, so simply use the constructor
function in the PyTypeObject 'tp_new' slot of data->newargs.

The 'if (inst)' check after this doesn't hurt in as much as it prevented
a segfault immediately after this failed allocation, but it doesn't help
much since the null pointer dereference will probably happen sooner or
later anyway.
@hfalcic
Copy link
Contributor Author

hfalcic commented Feb 15, 2014

Probably should have mentioned this in the commit message: this commit matches a change that I made to a SWIG-using project at https://github.com/hfalcic/librets/commit/74758d2b78b05ee8472bbc75c6fe11d851ee9783 (though note that this is a git-svn clone so this specific commit is subject to rebasing). During the librets build process, I patch the SWIG-generated C++ code before it's compiled.

Without this change, any kind of C++ exception (wrapped by a Python exception as a shadow instance) would segfault Python.

I'm rather inexperienced with SWIG and it would probably take me a while to create a minimal failing example, but I can try to do so if it would help.

@wsfulton
Copy link
Member

Can you run the SWIG test-suite on your machine? I think I've seen segfaults in Python 3 on some machines. If you can see segfaults in the test-suite before your patch and they go away after applying the patch, there is no need to produce an example. Otherwise an example would be much appreciated. Can you provide your operating system, 32/64bit and exact python version too please.

@hfalcic
Copy link
Contributor Author

hfalcic commented Feb 15, 2014

I've never run the SWIG test suite myself, but will try to do so soon. Since the Travis CI builds (presumably) pass without my change, I'm guessing that the test suite doesn't cover this. In the event that the test suite passes without my change, I'll attempt to come up with a small example, add it to the test suite, and maybe add such a change to this pull request by the end of this weekend.

In the mean time, I saw perfectly consistent segfaults without my change on Ubuntu 12.04 LTS with Python 3.2.3, Ubuntu 13.10 with Python "3.3.2+", and Ubuntu 14.04 (continuously updated development version) with Python 3.3.3 and 3.3.4. All 64-bit. With my change, no segfaults in any of those versions.

@ojwb
Copy link
Member

ojwb commented Feb 16, 2014

Aha, this probably explains segfaults when trying to throw wrapped exceptions I've been seen with Xapian's Python3 bindings. I'll try out this patch and report back.

@ojwb
Copy link
Member

ojwb commented Feb 16, 2014

This does indeed fix the crashes I've been seeing with Python3.

I'm testing on Debian unstable x86-64, Python 3.3.4-1.

Testsuite runs under python3 with and without the change shows this fixes several testcases:

checking testcase li_std_string_extra (with run test) under python
Traceback (most recent call last):
  File "./li_std_string_extra_runme3.py", line 24, in <module>
    s = s + "llo"
  File "/home/olly/git/swig/Examples/test-suite/python/li_std_string_extra.py", line 149, in __add__
    def __add__(self, *args) -> "std::basic_string< char,std::char_traits< char >,std::allocator< char > > *" : return _li_std_string_extra.string___add__(self, *args)
TypeError: object() takes no parameters
make[1]: *** [li_std_string_extra.cpptest] Error 1

Similarly fixed are li_std_wstring and testcase primitive_types.

And:

checking testcase exception_order (with run test) under python
Segmentation fault
make[1]: *** [exception_order.cpptest] Error 139

Similarly fixed is li_std_except_as_class.

There are no new failures.

@wsfulton
Copy link
Member

I've now added Python-3.3 testing in Travis which shows up the problem, see https://travis-ci.org/swig/swig/jobs/18986363. Two of the examples were also crashing:

checking Examples/python/exception
Segmentation fault (core dumped)
make[3]: *** [python_run] Error 139
make[2]: *** [check] Error 2
make[1]: *** [exception.actionexample] Error 2
checking Examples/python/exceptproxy
Traceback (most recent call last):
  File "/oldhome/william/swig/github/swig/Examples/python/exceptproxy/example.py", line 96, in __init__
    this = _example.new_FullError(*args)
TypeError: new_FullError() takes exactly 1 argument (0 given)
make[3]: *** [python_run] Error 1
make[2]: *** [check] Error 2
make[1]: *** [exceptproxy.actionexample] Error 2

wsfulton added a commit that referenced this pull request Feb 16, 2014
@wsfulton wsfulton merged commit c063bb8 into swig:master Feb 16, 2014
@hfalcic
Copy link
Contributor Author

hfalcic commented Feb 16, 2014

Thanks! I hadn't had time to work on test cases yet (or even run the test suite) and didn't realize that this might already be covered.

@wsfulton
Copy link
Member

@hfalcic thanks for the patch. The new Python 3.3 testing on Travis is now all green - https://travis-ci.org/swig/swig/jobs/18988875.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants