| msg332342 - (view) |
Author: Karthikeyan Singaravelan (xtreak) *  |
Date: 2018-12-22 10:20 |
| I was looking into format issues and came across . The example causes segfault in master, 3.7 and 3.6 branches. This used to pass in 3.7 and 3.6. I searched for open issues and cannot come across an issue for this. I guess this is caused due to which adds an assert as I can see from the segfault. Compiling in release mode works fine but debug build fails. Are asserts removed in release builds? $ python3.7 Python 3.7.1rc2 (v3.7.1rc2:6c06ef7dc3, Oct 13 2018, 05:10:29) [Clang 6.0 (clang-600.0.57)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> format(float(123), "00") '123.0' Master master $ ./python.exe Python 3.8.0a0 (heads/35559:c1b4b0f616, Dec 22 2018, 15:00:08) [Clang 7.0.2 (clang-700.1.81)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> format(float(123), "00") Assertion failed: (0 <= min_width), function _PyUnicode_InsertThousandsGrouping, file Objects/unicodeobject.c, line 9394. Python 3.6 cpython git:(5241ecff16) ./python.exe Python 3.6.8rc1+ (remotes/upstream/3.6:5241ecff16, Dec 22 2018, 15:05:57) [GCC 4.2.1 Compatible Apple LLVM 7.0.2 (clang-700.1.81)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> format(float(123), "00") Assertion failed: (0 <= min_width), function _PyUnicode_InsertThousandsGrouping, file Objects/unicodeobject.c, line 9486. [1] 33859 abort ./python.exe Python 3.7 cpython git:(c046d6b618) ./python.exe Python 3.7.2rc1+ (remotes/upstream/3.7:c046d6b618, Dec 22 2018, 15:07:24) [Clang 7.0.2 (clang-700.1.81)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> format(float(123), "00") Assertion failed: (0 <= min_width), function _PyUnicode_InsertThousandsGrouping, file Objects/unicodeobject.c, line 9369. [1] 42710 abort ./python.exe Latest master, 3.6 and 3.7 branch has this bug in debug mode with this being last Python 3.6 bug fix release. Commenting out the assert line gives me the correct result but I have limited knowledge of the C code and I guess release builds remove asserts where it cannot be reproduced? I am tagging devs who might have a better understanding of this and there might be limited bandwidth for someone to look into this along with other cases since it's holiday season. # Release mode works fine ./python.exe -c 'print(format(float(123), "00"))' 123.0 |
|
|
| msg332347 - (view) |
Author: Karthikeyan Singaravelan (xtreak) * |
Date: 2018-12-22 14:42 |
| Looking into the code min_width is returned as -2 and hence the assert fails. spec->n_min_width is passed as min_width to _PyUnicode_InsertThousandsGrouping that is used in the assert statement and I came across below comment that min_width can go negative and it's okay. So is the assert statement validating for it to be always greater than or equal to zero not needed? https://github.com/python/cpython/blob/59423e3ddd736387cef8f7632c71954c1859bed0/Python/formatter_unicode.c#L529 /* min_width can go negative, that's okay. format->width == -1 means we don't care. */ if (format->fill_char == '0' && format->align == '=') spec->n_min_width = format->width - n_non_digit_non_padding; else spec->n_min_width = 0; |
|
|
| msg332350 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-12-22 15:41 |
| Yes, seems the simplest way to fix this issue is to remove that assert. Do you mind to create a PR? Tests should be added to cover this case. |
|
|
| msg332353 - (view) |
Author: Karthikeyan Singaravelan (xtreak) * |
Date: 2018-12-22 15:58 |
| Sure, I will create one shortly. There were some other cases with different values of negative numbers that I will add since I couldn't see any tests failing on my debug builds. * Are there chances that bugs like these are present since I guess we use release builds in our CI? * Is this a release blocker for next RC since current set of 3.6.8 RC1 bugs fixes are complete I hope ? |
|
|
| msg332354 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-12-22 16:14 |
| This bug is not new, and this is the first report for it. It can be treated as a security issue if an application allows user to specify format string. But using a format string from untrusted source causes a security issue itself, because this allows to spend memory and CPU time for creating an arbitrary large string object. Also, unlikely debug builds be used in production. I would backport the solution of this issue to 3.6, but it is not bad if it will be not backported. I think this is not a release blocker. |
|
|
| msg332356 - (view) |
Author: Karthikeyan Singaravelan (xtreak) * |
Date: 2018-12-22 17:17 |
| > This bug is not new, and this is the first report for it. It can be treated as a security issue if an application allows user to specify format string. But using a format string from untrusted source causes a security issue itself, because this allows to spend memory and CPU time for creating an arbitrary large string object. Also, unlikely debug builds be used in production. My initial thought was that since the assert failed it has exposed some bug or behavior change. Also I didn't know release builds remove assert statements. Since it's a case of debug build being a problem I agree with you that impact is low since it shouldn't be used in production. > I would backport the solution of this issue to 3.6, but it is not bad if it will be not backported. I think this is not a release blocker. Thanks, I have created a PR with tests https://github.com/python/cpython/pull/11288 . For some reason it's not linked to the issue. |
|
|
| msg332359 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-12-22 17:56 |
| min_width сan be large negative number, and there are subtractions from it. It may be safer to replace the assert with something like min_width = Py_MAX(0, min_width). Or ensure that it is non-negative before calling _PyUnicode_InsertThousandsGrouping(). |
|
|
| msg332361 - (view) |
Author: Karthikeyan Singaravelan (xtreak) * |
Date: 2018-12-22 18:21 |
| > min_width сan be large negative number, and there are subtractions from it. It may be safer to replace the assert with something like min_width = Py_MAX(0, min_width). Or ensure that it is non-negative before calling _PyUnicode_InsertThousandsGrouping() Looking at the code the loop seems to operate on the assumption that min_width is >= 0 in the beginning with the assert statement until both remaining and min_width are negative to break out of the loop. Applying Py_MAX(0, min_width) causes no test failures but maybe I have missed adding a case where large negative min_width might have triggered other issue. |
|
|
| msg333162 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-01-07 15:09 |
| New changeset 3f7983a25a3d19779283c707fbdd5bc91b1587ef by Victor Stinner (Xtreak) in branch 'master': bpo-35560: Remove assertion from format(float, "n") (GH-11288) https://github.com/python/cpython/commit/3f7983a25a3d19779283c707fbdd5bc91b1587ef |
|
|
| msg333167 - (view) |
Author: miss-islington (miss-islington) |
Date: 2019-01-07 15:26 |
| New changeset 9a413faa8708e5c2df509e97d48f18685c198b24 by Miss Islington (bot) in branch '3.7': bpo-35560: Remove assertion from format(float, "n") (GH-11288) https://github.com/python/cpython/commit/9a413faa8708e5c2df509e97d48f18685c198b24 |
|
|
| msg333169 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-01-07 15:27 |
| Thanks Karthikeyan Singaravelan for the bug report and the fix! |
|
|
| msg380696 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2020-11-10 19:03 |
| This bug was introduced into the Python 3.6.8 "bugfix" release. https://github.com/python/cpython/pull/23231 will fix it if anyone needs that on modern 3.6. |
|
|
| msg380705 - (view) |
Author: Ned Deily (ned.deily) * |
Date: 2020-11-10 19:58 |
| New changeset dae5d728bc3f1d4039b64e4ec3a9036fd5d19587 by Miss Islington (bot) in branch '3.6': bpo-35560: Remove assertion from format(float, "n") (GH-11288) (GH-23231) https://github.com/python/cpython/commit/dae5d728bc3f1d4039b64e4ec3a9036fd5d19587 |
|
|