classification
Title: format(float(123), "00") causes segfault in debug builds
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eric.smith, mark.dickinson, miss-islington, serhiy.storchaka, vstinner, xtreak
Priority: normal Keywords: patch

Created on 2018-12-22 10:20 by xtreak, last changed 2019-01-07 15:27 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 11288 merged xtreak, 2018-12-22 17:17
PR 11458 merged miss-islington, 2019-01-07 15:09
Messages (11)
msg332342 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-12-22 10:20
I was looking into format issues and came across msg99839 . 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 issue33954 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 issue33954 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) * (Python triager) 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) * (Python committer) 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) * (Python triager) 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) * (Python committer) 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) * (Python triager) 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) * (Python committer) 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) * (Python triager) 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) * (Python committer) 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) * (Python committer) Date: 2019-01-07 15:27
Thanks Karthikeyan Singaravelan for the bug report and the fix!
History
Date User Action Args
2019-01-07 15:27:54vstinnersetstatus: open -> closed
versions: - Python 3.6
messages: + msg333169

resolution: fixed
stage: patch review -> resolved
2019-01-07 15:26:22miss-islingtonsetnosy: + miss-islington
messages: + msg333167
2019-01-07 15:09:56miss-islingtonsetpull_requests: + pull_request10930
2019-01-07 15:09:17vstinnersetmessages: + msg333162
2018-12-22 18:21:24xtreaksetmessages: + msg332361
2018-12-22 18:09:44mark.dickinsonsetnosy: + mark.dickinson
2018-12-22 17:56:17serhiy.storchakasetmessages: + msg332359
2018-12-22 17:17:09xtreaksetkeywords: + patch

stage: patch review
messages: + msg332356
pull_requests: + pull_request10513
2018-12-22 16:14:26serhiy.storchakasetmessages: + msg332354
2018-12-22 15:58:13xtreaksetmessages: + msg332353
2018-12-22 15:43:23serhiy.storchakasetcomponents: + Interpreter Core
2018-12-22 15:41:50serhiy.storchakasetmessages: + msg332350
2018-12-22 14:42:21xtreaksetmessages: + msg332347
2018-12-22 10:20:42xtreakcreate