Check that _PyUnicode_AsString() result is not NULL

@malemburg

BPO	6697
Nosy	@malemburg, @loewis, @ronaldoussoren, @amauryfa, @abalkin, @vstinner, @ezio-melotti
Files	test.py: test.py invalid_utf8_characters_from_command_line.py: crashers/invalid_utf8_characters_from_command_line.py issue6697.diff issue6697a.diff issue6697b.diff issue6697-lsprof.diff

^{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 = 'https://github.com/abalkin'
closed_at = <Date 2011-06-20.13:08:04.113>
created_at = <Date 2009-08-13.20:07:31.030>
labels = ['OS-mac', 'interpreter-core', 'expert-unicode', 'type-crash']
title = 'Check that _PyUnicode_AsString() result is not NULL'
updated_at = <Date 2011-06-20.13:08:04.112>
user = 'https://bugs.python.org/Arfrever'

bugs.python.org fields:

activity = <Date 2011-06-20.13:08:04.112>
actor = 'vstinner'
assignee = 'belopolsky'
closed = True
closed_date = <Date 2011-06-20.13:08:04.113>
closer = 'vstinner'
components = ['Interpreter Core', 'macOS', 'Unicode']
creation = <Date 2009-08-13.20:07:31.030>
creator = 'Arfrever'
dependencies = []
files = ['14711', '14713', '19958', '19970', '19972', '20397']
hgrepos = []
issue_num = 6697
keywords = ['patch', 'needs review']
message_count = 48.0
messages = ['91533', '91534', '91576', '91727', '91728', '91730', '91731', '91732', '91742', '100445', '100446', '100468', '100474', '100476', '100680', '100683', '100946', '100947', '100948', '101458', '105892', '105893', '106016', '106019', '106020', '106021', '106022', '106023', '106024', '106058', '123473', '123474', '123479', '123482', '123566', '123568', '123573', '123574', '123575', '123577', '123582', '123662', '123664', '123767', '126216', '126221', '138705', '138706']
nosy_count = 10.0
nosy_names = ['lemburg', 'loewis', 'jafo', 'ronaldoussoren', 'amaury.forgeotdarc', 'belopolsky', 'vstinner', 'ezio.melotti', 'Arfrever', 'python-dev']
pr_nums = []
priority = 'high'
resolution = 'fixed'
stage = 'commit review'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue6697'
versions = ['Python 3.2', 'Python 3.3']

Python 3.1 segfaults when invalid UTF-8 characters are passed from
command line.

In BASH shell you can run:
$ python3.1 -c $'print("\x80")'
Segmentation fault

In other POSIX-compatible shells you can save the attached test.py
files in current directory and run:
$ python3.1 -c "$(<test.py)"
Segmentation fault

I'm attaching crashers/invalid_utf8_characters_from_command_line.py.

The error occurs in Py_Main(), on _PyUnicode_AsString(commandObj). The
problem is that _PyUnicode_AsString() is not checked for error. Here is
a patch fixing two errors:

display on error message instead of a crash on
_PyUnicode_AsString(commandObj) failure
don't call Py_DECREF(commandObj) if commandObj is NULL
(PyUnicode_FromWideChar error, a different error)

My patch also includes a test. The test should be moved somewhere else,
but I don't know where.

The problem is actually wider::
    >>> getattr(None, "\udc80")
    Segmentation fault
An idea would be to change _PyUnicode_AsDefaultEncodedString and allow
unpaired surrogates (utf8+surrogateescape, as explained in PEP383), but
I fear the consequences...

The code that fails seems pretty common:
PyErr_Format(PyExc_AttributeError,
"'%.50s' object has no attribute '%.400s'",
tp->tp_name, _PyUnicode_AsString(name));
It would be unfortunate to replace all usages of _PyUnicode_AsString to
check the return value.

Martin, what do you think?

Amaury Forgeot d'Arc wrote:
> 
> Amaury Forgeot d'Arc <amauryfa@gmail.com> added the comment:
> 
> The problem is actually wider::
>     >>> getattr(None, "\udc80")
>     Segmentation fault
> An idea would be to change _PyUnicode_AsDefaultEncodedString and allow
> unpaired surrogates (utf8+surrogateescape, as explained in PEP383), but
> I fear the consequences...
>
> The code that fails seems pretty common:
> 	PyErr_Format(PyExc_AttributeError,
> 		     "'%.50s' object has no attribute '%.400s'",
> 		     tp->tp_name, _PyUnicode_AsString(name));
> It would be unfortunate to replace all usages of _PyUnicode_AsString to
> check the return value.

The use of _PyUnicode_AsString() is wrong here. There are several
cases where it can fail, e.g. MemoryErrors, embedded NULs, encoding
errors.

The same is true for _PyUnicode_AsStringAndSize(), which is why
I turned them into Python interpreter private APIs before 3.0
shipped.

If you want a fail-safe stringified version of a Unicode object,
your only choice is to create a new API that does error checking,
properly clears the error and then returns a reference to a constant
string, e.g. "<repr-error>".

Do you suggest to remove all usages of _PyUnicode_AsString() and
_PyUnicode_AsStringAndSize()?

Amaury Forgeot d'Arc wrote:

Amaury Forgeot d'Arc <amauryfa@gmail.com> added the comment:

Do you suggest to remove all usages of _PyUnicode_AsString() and
_PyUnicode_AsStringAndSize()?

In the short-term, I suggest that all uses that do not check the
return value get replaced with a new API which implements a failsafe
return value strategy.

In the mid- to long-term, the APIs should probably be removed
altogether.

They look a lot like the PyString APIs using the same names, but unlike
those APIs, they can fail, so the implied straight-forward conversion
of the PyString APIs to the above APIs gives a wrong impression to the
developers.

For error messages, I'd use the repr() of the objects - lone UTF-8
surrogates will not work since they cause issues further down the line
with debugging tools or even stderr terminal displays.

The %U format seems adequate for this purpose - actually
PyObject_GenericSetAttr uses it already.

Yes, the exception message will contain the same lone UTF-8
surrogates; this is not a problem because sys.stderr uses the
"backslashreplace" error handler.

It would be unfortunate to replace all usages of _PyUnicode_AsString to
check the return value.

I agree with MAL: we do need to check for errors returned from
_PyUnicode_AsString, and it would be best if we created a fail-safe
version of it.

In the specific case (getattr), it might also be useful to create a
result that is unicode-escaped, i.e. with \u escapes for all non-ASCII
non-printable characters.

For _PyUnicode_AsString, I'm uncertain whether supporting half
surrogates is a good idea. Unless there is a compelling reason to
support them, I think we leave that as-is. Your example is not
compelling: I think the unicode string should be escaped, anyway.

The OP's case is also not compelling, we should print an error
message that the source code is incorrectly encoded.

Here is a fix for object.c (object_pyunicode_asstring-py3k.patch):

PyObject_GenericGetAttr(): Replace PyErr_Format("... %.400s", ..., _PyUnicode_AsString(name)) by PyErr_Format("... %U", ..., name), as done in PyObject_GenericSetAttr(). Note that the string will no more be truncated to 400 bytes
PyObject_GetAttr(), PyObject_SetAttr(): Catch _PyUnicode_AsString() error

It fixes the crash getattr(1, "\uDAD1\uD51E") (used as a test in the patch).

Alexander Belopolsky wrote:

Alexander Belopolsky <belopolsky@users.sourceforge.net> added the comment:

I am attaching a revised version of the patch which also includes some tests. Interestingly, the issue in syslog module is a regression from 3.1 introduced in r80401. Although it is not a crasher, I don't think it was intentional because although openlog() is happy to accept NULL for indent, the error from _PyUnicode_AsString() would have to be cleared if the intent was to ignore undecodable indent.

Some notes:

Rather than just patching in error handling code, please consider
removing use of those APIs and replace their calls with something
more appropriate, e.g. using a parser API.
When ignoring errors from the API, you have to clear the exception.
This is missing in a couple of places in the patch, e.g. in pyexpat.c
Please also remove hacks like these:

+#define CMP PyUnicode_CompareWithASCIIString
+ if (CMP(nameobj, "entity") == 0)
+ res = self->entity;
+ else if (CMP(nameobj, "target") == 0)
+ res = self->target;
+ else if (CMP(nameobj, "version") == 0) {
+ return PyUnicode_FromFormat(
+ "Expat %d.%d.%d", XML_MAJOR_VERSION,
XML_MINOR_VERSION, XML_MICRO_VERSION);

On Tue, Dec 7, 2010 at 12:44 PM, Marc-Andre Lemburg
<report@bugs.python.org> wrote:
..

Rather than just patching in error handling code, please consider
removing use of those APIs and replace their calls with something
more appropriate, e.g. using a parser API.

Yes, that's what I started doing in the "a" patch. I am not sure what
you mean by "a parser API." There are several places where conversion
is either unnecessary or an encoded string is already available. See
_elementtree.c.

When ignoring errors from the API, you have to clear the exception.
This is missing in a couple of places in the patch, e.g. in pyexpat.c

Right. On the other hand, this is very similar to xmlparser_getattro
in _elementtree.c and I think should be handled the same way.

Please also remove hacks like these:

+#define CMP PyUnicode_CompareWithASCIIString

if (CMP(nameobj, "entity") == 0)

What do you consider a hack? The use of
PyUnicode_CompareWithASCIIString() or the shortening macro?

Alexander Belopolsky wrote:

Alexander Belopolsky <belopolsky@users.sourceforge.net> added the comment:

On Tue, Dec 7, 2010 at 12:44 PM, Marc-Andre Lemburg
<report@bugs.python.org> wrote:
..
> * Rather than just patching in error handling code, please consider
> removing use of those APIs and replace their calls with something
> more appropriate, e.g. using a parser API.
>
Yes, that's what I started doing in the "a" patch. I am not sure what
you mean by "a parser API."

PyArg_Parse() et al. See the discussion earlier on this ticket.

There are several places where conversion
is either unnecessary or an encoded string is already available. See
_elementtree.c.

If the API is not needed at all, even better.

> * When ignoring errors from the API, you have to clear the exception.
> This is missing in a couple of places in the patch, e.g. in pyexpat.c
>

Right. On the other hand, this is very similar to xmlparser_getattro
in _elementtree.c and I think should be handled the same way.

Not sure what you mean here. If you ignore errors and don't clear
the exception, it will pop up at some later point in processing
and that's generally very confusing.

> * Please also remove hacks like these:
>
> +#define CMP PyUnicode_CompareWithASCIIString
> + if (CMP(nameobj, "entity") == 0)

What do you consider a hack? The use of
PyUnicode_CompareWithASCIIString() or the shortening macro?

The shortening macro.

On Tue, Dec 7, 2010 at 1:11 PM, Marc-Andre Lemburg
<report@bugs.python.org> wrote:

> I am not sure what
> you mean by "a parser API."

PyArg_Parse() et al. See the discussion earlier on this ticket.

I've just realized that. It is the "u#" code. Yes, I'll see if I can
use it instead of "U", but I think in the affected code the PyUnicode
object is needed as well.

> .. this is very similar to xmlparser_getattro
> in _elementtree.c and I think should be handled the same way.

Not sure what you mean here. If you ignore errors and don't clear
the exception, it will pop up at some later point in processing
and that's generally very confusing.

I mean not converting to char* at all and use
PyUnicode_CompareWithASCIIString() instead of strcmp(). I wish that
function had a shorter name, though, but it is not a big deal to spell
it out. BTW, I don't think there is a way to use wchar_t* literals in
Python code, right? As in Py_UNICODE_strcmp(name, L"version").

Alexander Belopolsky wrote:
> 
> Alexander Belopolsky <belopolsky@users.sourceforge.net> added the comment:
> 
> On Tue, Dec 7, 2010 at 1:11 PM, Marc-Andre Lemburg
> <report@bugs.python.org> wrote:
>>>  I am not sure what
>>> you mean by "a parser API."
>>
>> PyArg_Parse() et al. See the discussion earlier on this ticket.
>>
> 
> I've just realized that.  It is the "u#" code.  Yes, I'll see if I can
> use it instead of "U", but I think in the affected code the PyUnicode
> object is needed as well.
> 
>>> ..  this is very similar to xmlparser_getattro
>>> in _elementtree.c and I think should be handled the same way.
>>
>> Not sure what you mean here. If you ignore errors and don't clear
>> the exception, it will pop up at some later point in processing
>> and that's generally very confusing.
>>
> 
> I mean not converting to char* at all and use
> PyUnicode_CompareWithASCIIString() instead of strcmp().  I wish that
> function had a shorter name, though, but it is not a big deal to spell
> it out.

Agreed; not my invention ;-) I would have used
PyUnicode_CompareToUTF8() or something along those lines.

BTW, I don't think there is a way to use wchar_t* literals in
Python code, right? As in Py_UNICODE_strcmp(name, L"version").

No, since wchar_t may be something completely different than
Py_UNICODE.

No sure about today's situation, but at least a couple of years
ago wchar_t was not defined on all supported platforms, e.g.
Crays didn't have it.

issue6697b.diff addresses Marc's comments. Thanks for the review.

Committed revision 87137. Needs backporting. Also as Victor suggested, _lsprof.c code can be refactored to avoid roundtrips of unicode through utf8 char*.

I am attaching an untested rewrite of normalizeUserObj() in _lsprof.c for comments on whether it is worth the effort. There might be other places where PyModule_GetName() can be profitably replaced with PyModule_GetNameObject().

bpo-6697-lsprof.diff:

Oh, I did recently a similar change on PyModule: I created PyModule_GetFilenameObject()
"PyObject * mod" => "PyObject *mod"
modname is not initialized if fn->m_module (mod) is NULL => initialize modname to NULL
there is a reference leak (modname)

I am replacing bpo-6697-lsprof.diff with a (hopefully) more carefully written version that addresses the issues that Victor noted.

Victor,

I take your comment as +1 for adding PyModule_GetNameObject().

I started looking into adding unit tests that would exercise this code, but is does not seem possible to trigger the fn->m_self == NULL condition. According to the comment in the code, this is supposed to be the case when fn is a builtin function, but I observe the following in the debugger when running test_cprofile:

(gdb) pyo fn
object : <built-in function exec>
type : builtin_function_or_method
refcount: 4
address : 0x10038c678
$5 = void
(gdb) pyo fn->m_self
object : <module 'builtins' (built-in)>
type : module
refcount: 51
address : 0x100388ee8
$6 = void

Le jeudi 13 janvier 2011 à 23:05 +0000, Alexander Belopolsky a écrit :

I take your comment as +1 for adding PyModule_GetNameObject().

I wrote a similar patch to add PyModule_GetNameObject() (I am working on
another huge patch, to fix bpo-3080). You have to document the new function
in Doc/c-api/module.rst.

Yes, it's better to work on unicode than encode unicode to bytes
(PyModule_GetName() with UTF-8) and then decode bytes from unicode
(PyUnicode_FromFormat with %s).

I am too tired to review the patch.

New changeset b87eac0369b5 by Victor Stinner in branch 'default':
Issue bpo-6697: _lsprof: normalizeUserObj() doesn't encode/decode (UTF-8) the
http://hg.python.org/cpython/rev/b87eac0369b5

I wrote a similar patch to add PyModule_GetNameObject()
(I am working on another huge patch, to fix bpo-3080)

Issue bpo-3080 added the PyModule_GetNameObject() function, so it simplify your patch.

I commited your bpo-6697-lsprof.diff patch, I just fixed a refleak (if modname is "builtins").

I want to close this generic issue. I think that we fixed enough code. If you still see code not checking that _PyUnicode_AsString() result is not NULL, please open a new specific issue.

Navigation Menu

Repository navigation

Check that _PyUnicode_AsString() result is not NULL #50946

29 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Check that _PyUnicode_AsString() result is not NULL #50946

Description

Activity

Arfrever commented on Aug 13, 2009

Arfrever commented on Aug 13, 2009

vstinner commented on Aug 14, 2009

amauryfa commented on Aug 19, 2009

malemburg commented on Aug 19, 2009

amauryfa commented on Aug 19, 2009

malemburg commented on Aug 19, 2009

amauryfa commented on Aug 19, 2009

loewis commented on Aug 19, 2009

vstinner commented on Mar 5, 2010

29 remaining items

malemburg commented on Dec 7, 2010

abalkin commented on Dec 7, 2010

malemburg commented on Dec 7, 2010

abalkin commented on Dec 7, 2010

malemburg commented on Dec 7, 2010

abalkin commented on Dec 7, 2010

abalkin commented on Dec 8, 2010

abalkin commented on Dec 9, 2010

vstinner commented on Dec 11, 2010

abalkin commented on Jan 13, 2011

vstinner commented on Jan 14, 2011

python-dev commented on Jun 20, 2011

vstinner commented on Jun 20, 2011

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions