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

a SystemError and an assertion failure in warnings.warn_explicit() #75466

Closed
orenmn mannequin opened this issue Aug 26, 2017 · 18 comments
Closed

a SystemError and an assertion failure in warnings.warn_explicit() #75466

orenmn mannequin opened this issue Aug 26, 2017 · 18 comments
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@orenmn
Copy link
Mannequin

orenmn mannequin commented Aug 26, 2017

BPO 31285
Nosy @brettcannon, @ncoghlan, @ericsnowcurrently, @serhiy-storchaka, @orenmn
PRs
  • bpo-31285: fix an assertion failure and a SystemError in warnings.warn_explicit #3219
  • [3.6] bpo-31285: Fix an assertion failure and a SystemError in warnings.warn_explicit. (GH-3219) #3775
  • bpo-31285: Remove splitlines identifier from Python/_warnings.c (forgot to remove it in #3219) #3803
  • [2.7] bpo-31285: fix an assertion failure and a SystemError in warnings.warn_explicit (GH-3219) #3805
  • [2.7] bpo-31285: fix an assertion failure and a SystemError in warnings.warn_explicit (GH-3219) #3823
  • [3.6] bpo-31285: Remove splitlines identifier from Python/_warnings.c (GH-3803) #3829
  • 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-09-30.14:07:34.412>
    created_at = <Date 2017-08-26.18:48:22.023>
    labels = ['interpreter-core', '3.7', 'type-crash']
    title = 'a SystemError and an assertion failure in warnings.warn_explicit()'
    updated_at = <Date 2017-09-30.14:07:34.410>
    user = 'https://github.com/orenmn'

    bugs.python.org fields:

    activity = <Date 2017-09-30.14:07:34.410>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-09-30.14:07:34.412>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2017-08-26.18:48:22.023>
    creator = 'Oren Milman'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31285
    keywords = ['patch']
    message_count = 18.0
    messages = ['300892', '300900', '300940', '300950', '302876', '303092', '303258', '303260', '303262', '303298', '303300', '303303', '303306', '303308', '303349', '303350', '303353', '303410']
    nosy_count = 5.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'eric.snow', 'serhiy.storchaka', 'Oren Milman']
    pr_nums = ['3219', '3775', '3803', '3805', '3823', '3829']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue31285'
    versions = ['Python 2.7', 'Python 3.6', 'Python 3.7']

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Aug 26, 2017

    the following causes an assertion failure in Python/_warnings.c in
    show_warning():

    import warnings
    
    class BadLoader:
        def get_source(self, fullname):
            class BadSource:
                def splitlines(self):
                    return [42]
            return BadSource()

    del warnings._showwarnmsg
    warnings.warn_explicit(message='foo', category=ArithmeticError, filename='bar',
    lineno=1, module_globals={'__loader__': BadLoader(),
    '__name__': 'foobar'})

    in short, the assertion failure would happen in warnings.warn_explicit() in case
    module_globals['__loader__'].get_source(module_globals['__name__']).splitlines()[lineno-1]
    is not a str.

    the following raises a SystemError:

    import warnings
    
    class BadLoader:
        def get_source(self, fullname):
            class BadSource:
                def splitlines(self):
                    return 42
            return BadSource()

    warnings.warn_explicit(message='foo', category=UserWarning, filename='bar',
    lineno=42, module_globals={'__loader__': BadLoader(),
    '__name__': 'foobar'})

    in short, warnings.warn_explicit() raises the SystemError in case
    module_globals['__loader__'].get_source(module_globals['__name__']).splitlines()
    is not a list.

    ISTM that adding a check in warnings_warn_explicit() (in Python/_warnings.c),
    to check whether
    module_globals['__loader__'].get_source(module_globals['__name__'])
    is a str (after existing code found out that it isn't None) would prevent both
    the assertion failure and the SystemError.

    What do you think? Is it OK to permit get_source() to return only None or a str?

    @orenmn orenmn mannequin added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Aug 26, 2017
    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Aug 26, 2017

    on a second thought, BadSource could be a subclass of str, so maybe we
    should just check whether
    module_globals['__loader__'].get_source(module_globals['__name__']).splitlines()[lineno-1]
    is a str,
    and whether
    module_globals['__loader__'].get_source(module_globals['__name__']).splitlines()
    is a list.

    @serhiy-storchaka
    Copy link
    Member

    An alternative solution is using str.splitlines(source) instead of source.splitlines(). It raises a TypeError if the argument is not a text string and always returns a list of strings.

    According to the documentation get_source() must return a text string or None.

    https://docs.python.org/3/library/importlib.html?highlight=get_source#importlib.abc.InspectLoader.get_source

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Aug 28, 2017

    ISTM that your solution is better than mine, Serhiy, so I updated the PR.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 91fb0af by Serhiy Storchaka (Oren Milman) in branch 'master':
    bpo-31285: Fix an assertion failure and a SystemError in warnings.warn_explicit. (bpo-3219)
    91fb0af

    @serhiy-storchaka
    Copy link
    Member

    New changeset 90fe25a by Serhiy Storchaka in branch '3.6':
    [3.6] bpo-31285: Fix an assertion failure and a SystemError in warnings.warn_explicit. (GH-3219) (bpo-3775)
    90fe25a

    @serhiy-storchaka
    Copy link
    Member

    PyUnicode_Splitlines() cannot be used here in 2.7, because the source is likely a 8-bit string.

    Actually I'm not sure this issue is worth to be fixed in 2.7. The fix would be different from 3.x and more complex.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Sep 28, 2017

    In 2.7, PyUnicode_Splitlines() first does:
    string = PyUnicode_FromObject(string);

    So i thought that PyUnicode_Splitlines() would be fine with receiving a string.

    But now i realize that even in case i was right there, PyUnicode_Splitlines()
    returns a unicode, and not a string, so there should be problems later.
    I wonder how the tests still passed..

    @serhiy-storchaka
    Copy link
    Member

    In 2.7, PyUnicode_Splitlines() first does:
    string = PyUnicode_FromObject(string);

    And it raises an exception if the string contains non-ASCII characters.

    It is better to avoid str<->unicode convertion as long as possible. And when
    do it for the output a warning, use "replace" or "backslashreplace" error
    handlers or "latin1" decoder to avoid a failure.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Sep 29, 2017

    Another thought - the existing code assumes that splitlines() returned a string.
    So maybe we could just check that get_source() returned a string, and then call
    the method str.splitlines() on it?

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Sep 29, 2017

    oh, of course, checking that get_source() returned a string before passing it to
    str.splitlines() is not needed.

    @serhiy-storchaka
    Copy link
    Member

    What if get_source() returned a unicode string? Usually it returns 8-bit string, but in many cases unicode is accepted if str is expected, so you need to check this option too.

    @orenmn
    Copy link
    Mannequin Author

    orenmn mannequin commented Sep 29, 2017

    But in case get_source() returned a unicode, is it likely that the splitlines() method
    of this unicode would return a 8-bit string? Currently show_warning() doesn't handle this
    scenario, as it assumes splitlines() returned an 8-bit string. Or do you think that
    show_warning() should also accept unicode?

    @serhiy-storchaka
    Copy link
    Member

    I didn't check. I supposed that the other code can support unicode by
    implicitly converting it to str. But now I see that show_warning() uses
    PyString_AS_STRING() and therefore supports only str. I agree, using
    str.splitlines() would be correct solution.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 8b4ff53 by Serhiy Storchaka (Oren Milman) in branch 'master':
    bpo-31285: Remove splitlines identifier from Python/_warnings.c (bpo-3803)
    8b4ff53

    @serhiy-storchaka
    Copy link
    Member

    I just missed PR 3803.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 66c2b9f by Serhiy Storchaka (Miss Islington (bot)) in branch '3.6':
    [3.6] bpo-31285: Remove splitlines identifier from Python/_warnings.c (GH-3803) (bpo-3829)
    66c2b9f

    @serhiy-storchaka
    Copy link
    Member

    New changeset 40d736b by Serhiy Storchaka (Oren Milman) in branch '2.7':
    [2.7] bpo-31285: Don't raise a SystemError in warnings.warn_explicit() in case __loader__.get_source() has a bad splitlines() method. (GH-3219) (bpo-3823)
    40d736b

    @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 interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant