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

[orenmn]

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]

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]

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]

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]

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

[serhiy-storchaka]

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]

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]

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]

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]

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]

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]

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

[serhiy-storchaka]

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.