Navigation Menu

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

assertion failure in random.seed() in case the seed argument has a bad __abs__() method #75659

Closed
orenmn mannequin opened this issue Sep 14, 2017 · 13 comments
Closed

assertion failure in random.seed() in case the seed argument has a bad __abs__() method #75659

orenmn mannequin opened this issue Sep 14, 2017 · 13 comments
Labels
3.7 (EOL) end of life extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@orenmn
Copy link
Mannequin

orenmn mannequin commented Sep 14, 2017

BPO 31478
Nosy @rhettinger, @serhiy-storchaka, @vedgar, @orenmn
PRs
  • bpo-31478: Fix an assertion failure in random.seed() in case the 'a' arg has a bad __abs__() method #3596
  • [3.6] bpo-31478: Fix an assertion failure in random.seed() in case a … #3794
  • [2.7] bpo-31478: Prevent unwanted behavior in _random.Random.seed() in case the arg has a bad __abs__() method (GH-3596) #3845
  • 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 2017-10-02.21:35:15.517>
    created_at = <Date 2017-09-14.20:33:46.293>
    labels = ['extension-modules', '3.7', 'type-crash']
    title = 'assertion failure in random.seed() in case the seed argument has a bad __abs__() method'
    updated_at = <Date 2017-10-02.21:35:15.516>
    user = 'https://github.com/orenmn'

    bugs.python.org fields:

    activity = <Date 2017-10-02.21:35:15.516>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-10-02.21:35:15.517>
    closer = 'serhiy.storchaka'
    components = ['Extension Modules']
    creation = <Date 2017-09-14.20:33:46.293>
    creator = 'Oren Milman'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31478
    keywords = ['patch']
    message_count = 13.0
    messages = ['302208', '302228', '302232', '302234', '302241', '302317', '302740', '302782', '303196', '303201', '303207', '303451', '303566']
    nosy_count = 4.0
    nosy_names = ['rhettinger', 'serhiy.storchaka', 'veky', 'Oren Milman']
    pr_nums = ['3596', '3794', '3845']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue31478'
    versions = ['Python 2.7', 'Python 3.6', 'Python 3.7']

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Sep 14, 2017

    The following code causes an assertion failure:
    class BadInt(int):
    def __abs__(self):
    return None

    import random
    
    random.seed(BadInt())

    this is because random_seed() (in Modules/_randommodule.c) assumes that
    PyNumber_Absolute() returned an int, and so it passes it to _PyLong_NumBits(),
    which asserts it received an int.

    what should we do in such a case?
    should we raise an exception? (the docs don't mention abs() in case the seed is
    an int - https://docs.python.org/3.7/library/random.html#random.seed)

    @orenmn orenmn mannequin added 3.7 (EOL) end of life extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump labels Sep 14, 2017
    @rhettinger
    Copy link
    Contributor

    It would make sense to verify that an actual int was returned and to raise a TypeError if it wasn't.

    Do you want to submit a PR (with a test case)?

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Sep 15, 2017

    sure.
    but what about the TypeError message? should it complain about
    the return value of abs(seed)? (the docs of random.seed don't mention abs().)

    @serhiy-storchaka
    Copy link
    Member

    There are several ways of solving this issue:

    1. Verify that an actual int was returned and raise a TypeError if it wasn't.

    2. Verify that an actual int was returned and fall back to the hash if it wasn't.

    3. Use int.__abs__() instead of abs(), the result will be guaranteed int. The drawback is that this makes a copy of positive integers for int subclasses.

    4. Check the sign of the seed and call int.__neg__() for negative values.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Sep 15, 2017

    i opened a PR that implements the first option, but of course I wouldn't
    mind if you decide another option is better.

    @vedgar
    Copy link
    Mannequin

    vedgar mannequin commented Sep 16, 2017

    So floats (and complexes) cannot be seeds anymore? :-o Or this pertains only to ints? In this case, I think the easiest doc fix is to change

    If a is an int, it is used directly.

    to

    If a is an int, its absolute value is used.

    @serhiy-storchaka
    Copy link
    Member

    The more I think about this the more I like the idea of using int.__abs__() directly (PyLong_Type.tp_as_number->nb_absolute() in C). The C code doesn't call potentially overridable methods bit_length() and to_bytes(), it uses concrete implementations directly. I don't see reasons why it should obey overriding the __abs__() method.

    This will save us from the problem with the wording of the error message.

    I mentioned a drawback, but the current implementation has the same drawback. We can avoid copying positive integer subtypes by using more complex code, but I think it isn't worth.

    @rhettinger
    Copy link
    Contributor

    I don't see reasons why it should obey overriding the __abs__() method.

    I concur.

    @serhiy-storchaka
    Copy link
    Member

    New changeset d780b2d by Serhiy Storchaka (Oren Milman) in branch 'master':
    bpo-31478: Fix an assertion failure in random.seed() in case a seed has a bad __abs__() method. (bpo-3596)
    d780b2d

    @serhiy-storchaka
    Copy link
    Member

    New changeset befc956 by Serhiy Storchaka in branch '3.6':
    [3.6] bpo-31478: Fix an assertion failure in random.seed() in case a seed has a bad __abs__() method. (GH-3596) (bpo-3794)
    befc956

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Sep 28, 2017

    With regard to backporting to 2.7:

    In 2.7 also, PyNumber_Absolute() is called, and its return value is stored in
    the variable n.
    However, there is no _PyLong_NumBits(n), so there is no assertion failure.
    If n isn't an integer:

    • if !PyObject_IsTrue(n), then the seed is zero (e.g. if n is None, [], () or {})
    • otherwise, PyNumber_And() and PyNumber_Rshift() are used in a loop on n, so
      probably a TypeError would be raised.

    So I think a backport is still desirable, but i am not sure about the test.

    Maybe we should use @cpython_only, and make sure that no error is raised?
    We can also make sure that random() returns a different value than when the
    seed is zero.

    What do you think?

    @serhiy-storchaka
    Copy link
    Member

    I agree that a backport is still desirable.

    Your plan LGTM. Implement __abs__ raising an exception (test both int ant long subclasses). Make sure that no error is raised. Make sure that random() returns the same value as when seeding with an exact int and long.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 13da1a6 by Serhiy Storchaka (Oren Milman) in branch '2.7':
    [2.7] bpo-31478: Prevent unwanted behavior in _random.Random.seed() in case the arg has a bad __abs__() method (GH-3596) (bpo-3845)
    13da1a6

    @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 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