wrapt icon indicating copy to clipboard operation
wrapt copied to clipboard

segfault when using ObjectProxy in pow

Open DRMacIver opened this issue 8 years ago • 10 comments

The following code triggers a segfault when run with the latest version of wrapt from develop:

import wrapt

pow(*map(wrapt.ObjectProxy, [1, 2, 3]))

I spotted this because I was looking through the C code after your fix for #106 and noticed that WraptObjectProxy_power did not seem to be unwrapping its modulo argument and thought I'd check what happened.

DRMacIver avatar Sep 15 '17 13:09 DRMacIver

I am not sure it should be attempting to unwrap the modulo. Even so, pow() should not crash. It should be raising an exception it not given a value of the correct type. Will need to dig into a bit. The docs on nb_power aren't very useful.

GrahamDumpleton avatar Sep 15 '17 13:09 GrahamDumpleton

I am not sure it should be attempting to unwrap the modulo.

Really? Why not? This might just be my misunderstanding matters, but I'd expect that if you had an object proxy wrapping a number you'd need to unwrap it if you wanted to use it as the modulo argument.

And yeah, regardless of whether unwrapping is correct, a segfault is obviously the wrong behaviour. Note that pow(1, 2, 3) is not an error, it just returns 1.

DRMacIver avatar Sep 15 '17 13:09 DRMacIver

All I can is that ternary operators are a mess.

/*
  Calling scheme used for ternary operations:

  Order operations are tried until either a valid result or error:
    v.op(v,w,z), w.op(v,w,z), z.op(v,w,z)
 */

So yes, it seems I may have to unwrap it as can fall through to last case.

GrahamDumpleton avatar Sep 15 '17 14:09 GrahamDumpleton

All I can is that ternary operators are a mess.

Yeah, operator resolution is already my least favourite part of Python even before you get into C extensions and ternary operators.

DRMacIver avatar Sep 15 '17 14:09 DRMacIver

I don't see a crash. I see:

TypeError: unsupported operand type(s) for pow(): 'int', 'int', 'ObjectProxy'

in most cases.

But for pypy I get:

>       self.assertEqual(pow(3, 2, two), pow(3, 2, 2))
E       TypeError: operands do not support **

I have seen that pypy behaviour around pow() is different previously.

        # Only PyPy implements __rpow__ for ternary pow().

        if is_pypy:
            self.assertEqual(pow(three, two, 2), pow(3, 2, 2))
            self.assertEqual(pow(3, two, 2), pow(3, 2, 2))

So long ago I forget what it was all about.

GrahamDumpleton avatar Sep 15 '17 14:09 GrahamDumpleton

Might be a Python version thing? This was on 3.6.0. Sorry, I should have said that up front.

DRMacIver avatar Sep 15 '17 14:09 DRMacIver

And how pure Python even does all the conversion I can't remember.

    def __pow__(self, other, *args):
        return pow(self.__wrapped__, other, *args)

I only unwrap the first argument on all the arithmetic dunderscore methods.

GrahamDumpleton avatar Sep 15 '17 14:09 GrahamDumpleton

So I've tried the following in a virtualenv for each of Python 2.7.13, 3.6.0 and 3.6.1 and I get a segfault in all three:

pip uninstall wrapt -y
WRAPT_INSTALL_EXTENSIONS=true pip install . --upgrade
python -c 'import wrapt; pow(*map(wrapt.ObjectProxy, [1, 2, 3]))'

All three segfault for me. This is with current develop. i.e. aaa932b4f71c4a1e5ebca2f5787cf9602616ae4f

Running this under faulthandler is completely uninformative I'm afraid.

Its possible this could be an oddity of my system - I'm running this on the Windows Subsystem for Linux - but that would be a surprising interaction.

DRMacIver avatar Sep 15 '17 14:09 DRMacIver

I may not have implemented the modulo conversion because it only works for C extension. There doesn't appear to be a way to have it work for pure Python code.

ERROR:   py26-without-extensions: commands failed
  py26-install-extensions: commands succeeded
ERROR:   py26-disable-extensions: commands failed
ERROR:   py27-without-extensions: commands failed
  py27-install-extensions: commands succeeded
ERROR:   py27-disable-extensions: commands failed
ERROR:   py33-without-extensions: commands failed
  py33-install-extensions: commands succeeded
ERROR:   py33-disable-extensions: commands failed
ERROR:   py34-without-extensions: commands failed
  py34-install-extensions: commands succeeded
ERROR:   py34-disable-extensions: commands failed
ERROR:   py35-without-extensions: commands failed
  py35-install-extensions: commands succeeded
ERROR:   py35-disable-extensions: commands failed
ERROR:   py36-without-extensions: commands failed
  py36-install-extensions: commands succeeded
ERROR:   py36-disable-extensions: commands failed
ERROR:   pypy-without-extensions: commands failed

So is not just pypy that is a problem.

For example, Python 2.7 results in:

>       self.assertEqual(pow(3, 2, two), pow(3, 2, 2))
E       TypeError: unsupported operand type(s) for pow(): 'int', 'int', 'ObjectProxy'

So I may at the time have punted and said using a proxy for modulo is not supported.

GrahamDumpleton avatar Sep 16 '17 04:09 GrahamDumpleton

Use of proxy for modulo doesn't even work in all cases when using C API either. Current thinking is that for nb_power case, shouldn't even unwrap the second argument, let alone modulo, thereby ensuring consistent behaviour, even if means can't pass proxy as second or third arguments.

Lets look at the combinations and how things work.

For pure Python code with CPython.

  • pow(Number, Number, Number) - No proxies involved.

  • pow(Number, Proxy, Number) - Fails with TypeError because pow() doesn't do type conversion on 2nd argument.

  • pow(Number, Number, Proxy) - Fails with TypeError because pow() doesn't do type conversion on 3rd argument.

  • pow(Proxy, Number, Number) - Will work as __pow__ in proxy can unwrap first argument.

  • pow(Proxy, Proxy, Number) - Fails with TypeError because current __pow__ in proxy doesn't unwrap 2nd argument and underlying pow() doesn't do type conversion on 2nd argument.

  • pow(Proxy, Number, Proxy) - Fails with TypeError because current __pow__ in proxy doesn't unwrap 3rd argument and underlying pow() doesn't do type conversion on 3rd argument.

For CPython C API.

  • pow(Number, Proxy, Number) - Fails with TypeError because pow() doesn't do type conversion on 2nd argument. Nothing in proxy is called.

  • pow(Number, Number, Proxy) - Fails with TypeError because pow() doesn't do type conversion on 3rd argument. Nothing in proxy is called.

  • pow(Proxy, Number, Number) - Will work as __pow__ in proxy can unwrap first argument.

  • pow(Proxy, Proxy, Number) - Currently works because proxy is unwrapping the 2nd argument before calling PyNumber_Power().

  • pow(Proxy, Number, Proxy) - Fails with TypeError because proxy currently doesn't unwrap 3rd argument and underlying PyNumber_Power doesn't do type conversion on 3rd argument.

For PyPy.

  • pow(Number, Number, Number) - No proxies involved.

  • pow(Number, Proxy, Number) - Fails with TypeError because pow() doesn't do type conversion on 2nd argument.

  • pow(Number, Number, Proxy) - Fails with TypeError because pow() doesn't do type conversion on 3rd argument.

  • pow(Proxy, Number, Number) - Will work as __pow__ in proxy can unwrap first argument.

  • pow(Proxy, Proxy, Number) - Fails with TypeError because current __pow__ in proxy doesn't unwrap 2nd argument and underlying pow() doesn't do type conversion on 2nd argument.

  • pow(Proxy, Number, Proxy) - Fails with TypeError because current __pow__ in proxy doesn't unwrap 3rd argument and underlying pow() doesn't do type conversion on 3rd argument.

  • pow(Number, Proxy, Number) - Will work as PyPy implements __rpow__ where as CPython doesn't, and proxy can unwrap second argument.

  • pow(Number, Proxy, Proxy) - Fails with TypeError because __rpow__ in proxy doesn't unwrap 3rd argument and underlying pow() doesn't do type conversion on 3rd argument.

The conclusion I am coming to is that for consistency I can't support unwrapping of 2nd or 3rd argument in CPython. This means changing the current C API code not to unwrap the second argument.

So for CPython, only thing that can be unwrapped is the first argument.

For PyPy, can still provide __rpow__ so it can unwrap second, but behaviour will then be different to CPython.

This is not going to be only case where manual unwrapping would be required. There are probably various builtin functions which don't support type conversion automatically.

GrahamDumpleton avatar Sep 17 '17 07:09 GrahamDumpleton