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

nb_inplace_pow is always called with an invalid argument #80560

Closed
Zuzu-Typ mannequin opened this issue Mar 20, 2019 · 11 comments
Closed

nb_inplace_pow is always called with an invalid argument #80560

Zuzu-Typ mannequin opened this issue Mar 20, 2019 · 11 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes build The build process and cross-build extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@Zuzu-Typ
Copy link
Mannequin

Zuzu-Typ mannequin commented Mar 20, 2019

BPO 36379
Nosy @rhettinger, @gpshead, @skrah, @MojoVampire, @ZackerySpytz, @miss-islington, @Zuzu-Typ
PRs
  • bpo-36379: __ipow__ must be a ternaryfunc, not a binaryfunc #13546
  • Files
  • Doesn't Work.png
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-01-27.23:14:19.150>
    created_at = <Date 2019-03-20.12:01:33.473>
    labels = ['extension-modules', '3.8', 'build', '3.7', 'type-crash']
    title = 'nb_inplace_pow is always called with an invalid argument'
    updated_at = <Date 2021-01-27.23:14:19.148>
    user = 'https://github.com/Zuzu-Typ'

    bugs.python.org fields:

    activity = <Date 2021-01-27.23:14:19.148>
    actor = 'gregory.p.smith'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-01-27.23:14:19.150>
    closer = 'gregory.p.smith'
    components = ['Build', 'Extension Modules']
    creation = <Date 2019-03-20.12:01:33.473>
    creator = 'Zuzu_Typ'
    dependencies = []
    files = ['48225']
    hgrepos = []
    issue_num = 36379
    keywords = ['patch']
    message_count = 11.0
    messages = ['338458', '338481', '338500', '338502', '338503', '338504', '339353', '343387', '344051', '375835', '385818']
    nosy_count = 7.0
    nosy_names = ['rhettinger', 'gregory.p.smith', 'skrah', 'josh.r', 'ZackerySpytz', 'miss-islington', 'Zuzu_Typ']
    pr_nums = ['13546']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue36379'
    versions = ['Python 2.7', 'Python 3.7', 'Python 3.8']

    @Zuzu-Typ
    Copy link
    Mannequin Author

    Zuzu-Typ mannequin commented Mar 20, 2019

    Using the C-API, the inplace_pow numbermethod is always called with the third argument pointing to an invalid address.

    The reason is likely that self.__ipow__ only takes one argument, resulting in a binaryfunc (self, arg), though inplace_pow is a ternaryfunc.
    When trying to use the third argument in any way, Python crashes.

    The third arg should be nonexistent, NULL or Py_None.

    @Zuzu-Typ Zuzu-Typ mannequin added 3.7 (EOL) end of life build The build process and cross-build extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump labels Mar 20, 2019
    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Mar 20, 2019

    object.__ipow__ is documented to take an optional third argument (though there is no way to pass it aside from explicitly calling __ipow__ directly since there is no syntax support for three-arg pow, in place or otherwise), so it's not some incompatibility with object.__ipow__'s signature.

    How are you seeing garbage passed? In the CPython C code base, I only see PyNumber_InPlacePower called in two places; ceval.c (to handle **=, which only handles two operands) and _operator.c (to implement operator.__ipow__, which unlike object.__ipow__, only takes two arguments, not three). In both cases, the third argument is explicitly passed in as Py_None.

    PyNumber_InPlacePower itself then passes along that third argument to ternary_op as its third argument, and every code path that calls the retrieved slot consistently passes that argument along as the third argument to the slotted ternaryfunc.

    I suppose an extension module might incorrectly call PyNumber_InPlacePower without passing the third argument, but that's a problem on their end (and should be caught by the compiler unless all diagnostics are suppressed).

    But I'm not seeing the problem here. The code path is probably untested (given all numeric types in the CPython core are immutable, so none of them set nb_inplace_pow), but it looks correct at first glance. Do you have code that reproduces the error?

    @Zuzu-Typ
    Copy link
    Mannequin Author

    Zuzu-Typ mannequin commented Mar 20, 2019

    Even though __ipow__ might be documented to take a third argument, if you build an inplace_pow function using the C-API, you can only pass one argument to it.

    You can see that in the attached screenshot.

    The example class shown in the screenshot can be found here: https://github.com/Zuzu-Typ/Python-C-API-extension-template

    With the little template I wasn't able to reproduce the crash, but I did reassure myself that the third object is neither Py_None nor NULL, by adding "if (obj2 == Py_None || obj2 == NULL) return NULL;" before line 469 in "template.c", because calling __ipow__ still returned an example_class instance, instead of an error message, as it should if it returned NULL.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Mar 20, 2019

    Like Josh I don't quite understand the problem description. This
    for example works:

    >>> class C(int):
    ...     def __ipow__(self, other, mod=None):
    ...         return pow(self, other, mod)
    ... 
    >>> 
    >>> x = C(10)
    >>> x
    10
    >>> x **= 3
    >>> x
    1000

    @Zuzu-Typ
    Copy link
    Mannequin Author

    Zuzu-Typ mannequin commented Mar 20, 2019

    This isn't about the CPython Interpreter, it's about the C-API, the APIT for writing c-extensions for Python.

    I know it works in CPython.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Mar 20, 2019

    Ok, got it. I think __ipow__ should be a ternaryfunc, like so:

    diff --git a/Objects/typeobject.c b/Objects/typeobject.c
    index 403f3caaee..914d076b5c 100644
    --- a/Objects/typeobject.c
    +++ b/Objects/typeobject.c
    @@ -7032,7 +7032,7 @@ static slotdef slotdefs[] = {
         IBSLOT("__imod__", nb_inplace_remainder, slot_nb_inplace_remainder,
                wrap_binaryfunc, "%="),
         IBSLOT("__ipow__", nb_inplace_power, slot_nb_inplace_power,
    -           wrap_binaryfunc, "**="),
    +           wrap_ternaryfunc, "**="),
         IBSLOT("__ilshift__", nb_inplace_lshift, slot_nb_inplace_lshift,
                wrap_binaryfunc, "<<="),
         IBSLOT("__irshift__", nb_inplace_rshift, slot_nb_inplace_rshift,

    On the other hand it is odd if "**=" can never use the third argument.

    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Apr 2, 2019

    skrah: Is there any reason your patch, as written, wouldn't work? If you need a test case to verify, gmpy2's xmpz type supports in place pow (but requires the modulus to be None, since there is no normal way to pass it anyway), so you can just test:

    >>> xm = gmpy2.xmpz(2)
    >>> xm.__ipow__(3, 5)
    

    Right now, that code will raise a TypeError (from check_num_args in wrap_binary_func):

    TypeError: expected 1 argument, got 2
    

    while:

    >>> xm.__ipow__(3)
    

    typically results in:

    SystemError: modulo not expected
    

    because wrap_binaryfunc fails to pass the expected argument so the receiver sees garbage, and xmpz's ipow implementation checks the third argument raises an exception if anything but None is received; barring a coincidence of Py_None being on the stack there, it'll always fail the test.

    Changing to wrap_ternaryfunc should make xm.__ipow__(3, 5) raise the SystemError currently raised by xm.__ipow__(3) (because it doesn't accept non-None), while xm.__ipow__(3) will work correctly.

    @MojoVampire MojoVampire mannequin added the 3.8 only security fixes label Apr 2, 2019
    @ZackerySpytz
    Copy link
    Mannequin

    ZackerySpytz mannequin commented May 24, 2019

    I've created a PR for this issue (with tests).

    @miss-islington
    Copy link
    Contributor

    New changeset c7f803b by Miss Islington (bot) (Zackery Spytz) in branch 'master':
    bpo-36379: __ipow__ must be a ternaryfunc, not a binaryfunc (GH-13546)
    c7f803b

    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Aug 24, 2020

    Zackery, should this be closed? Or is there something missing from the patch?

    @gpshead
    Copy link
    Member

    gpshead commented Jan 27, 2021

    I'm pretty sure this is fixed for 3.8+.

    whether or not it should be considered a bugfix and backported to 3.7.x is probably too late at this point in release lifecycles anyways.

    thanks for raising this and the fixing PR!

    @gpshead gpshead closed this as completed Jan 27, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes build The build process and cross-build extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants