bpo-34087: Fix buffer overflow in int(s) and similar functions by methane · Pull Request #8274 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation11 Commits4 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

methane

@methane

_PyUnicode_TransformDecimalAndSpaceToASCII() missed trailing NUL char. It cause buffer overflow in _Py_string_to_number_with_underscores().

This bug is introduced in bpo-31979, 9b6c60c.

@methane

serhiy-storchaka

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance of adding tests?

@methane

vstinner

@@ -391,6 +391,8 @@ _Py_string_to_number_with_underscores(
char *dup, *end;
PyObject *result;
assert(s[orig_len] == '\0');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this assertion. For performance, it would make sense to call _Py_string_to_number_with_underscores() on a sub-string which doesn't end with a NUL character. If you want to add an assertion, please do it at the caller site.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion is fine to me. _Py_string_to_number_with_underscores() is always called with a substring that end with a NUL character (if it doesn't end, we make a copy and call this function with it). And _Py_string_to_number_with_underscores() calls innerfunc() which expects a NUL terminated string.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, the following lines calls strchr() which expects that the string ends with a NUL byte.

In that case, should we also test that strlen(s) == orig_len to block embedded NUL byte?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not needed. NUL byte in string is valid.

vstinner

@@ -391,6 +391,8 @@ _Py_string_to_number_with_underscores(
char *dup, *end;
PyObject *result;
assert(s[orig_len] == '\0');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, the following lines calls strchr() which expects that the string ends with a NUL byte.

In that case, should we also test that strlen(s) == orig_len to block embedded NUL byte?

@@ -9072,6 +9072,7 @@ _PyUnicode_TransformDecimalAndSpaceToASCII(PyObject *unicode)
int decimal = Py_UNICODE_TODECIMAL(ch);
if (decimal < 0) {
out[i] = '?';
out[i+1] = '\0';
_PyUnicode_LENGTH(result) = i + 1;
break;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To debug other kinds of bugs, you can call "assert(_PyUnicode_CheckConsistency(unicode, 1));" just before "return result;". This function detects if the last character is not a NUL character, for example ;-)

@methane

@methane

vstinner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I tested manually and confirm that the 3 tests (test_complex test_float test_long) crash without the fix ("out[i+1] = '\0';"), and pass with the fix.

@vstinner

You might backport the new tests to 3.6 (it's up to you).

@miss-islington

Thanks @methane for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@bedevere-bot

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jul 14, 2018

@methane @miss-islington

…nGH-8274)

_PyUnicode_TransformDecimalAndSpaceToASCII() missed trailing NUL char. It caused buffer overflow in _Py_string_to_number_with_underscores().

This bug is introduced in 9b6c60c. (cherry picked from commit 16dfca4)

Co-authored-by: INADA Naoki methane@users.noreply.github.com

miss-islington added a commit that referenced this pull request

Jul 14, 2018

@miss-islington @methane

_PyUnicode_TransformDecimalAndSpaceToASCII() missed trailing NUL char. It caused buffer overflow in _Py_string_to_number_with_underscores().

This bug is introduced in 9b6c60c. (cherry picked from commit 16dfca4)

Co-authored-by: INADA Naoki methane@users.noreply.github.com

methane added a commit to methane/cpython that referenced this pull request

Jul 14, 2018

@methane

pythonGH-8276 fixed regression in 3.7. While the regression is not in 3.6, it's worth to backport test cases to 3.6 branch too.

methane added a commit that referenced this pull request

Jul 14, 2018

@methane

Cherrypick tests from 16dfca4

While the regression is not in 3.6, it's worth to backport test cases to 3.6 branch too.

CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request

Jul 14, 2018

@CuriousLearner

CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request

Jul 15, 2018

@CuriousLearner

CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request

Jul 21, 2018

@CuriousLearner

…ssue-33014

Labels

type-bug

An unexpected behavior, bug, or error