format(float(123), "00") causes segfault in debug builds

[tirkarthi]

BPO	35560
Nosy	@gpshead, @mdickinson, @vstinner, @ericvsmith, @ned-deily, @serhiy-storchaka, @miss-islington, @tirkarthi
PRs	bpo-35560: Remove incorrect assert statement that caused segfault in debug builds #11288 [3.7] bpo-35560: Remove assertion from format(float, "n") (GH-11288) #11458 [3.6] bpo-35560: Remove assertion from format(float, "n") (GH-11288) #23231

^{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 2019-01-07.15:27:54.657>
created_at = <Date 2018-12-22.10:20:42.854>
labels = ['interpreter-core', '3.7', '3.8', 'type-crash']
title = 'format(float(123), "00") causes segfault in debug builds'
updated_at = <Date 2020-11-10.19:58:35.740>
user = 'https://github.com/tirkarthi'

bugs.python.org fields:

activity = <Date 2020-11-10.19:58:35.740>
actor = 'ned.deily'
assignee = 'none'
closed = True
closed_date = <Date 2019-01-07.15:27:54.657>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2018-12-22.10:20:42.854>
creator = 'xtreak'
dependencies = []
files = []
hgrepos = []
issue_num = 35560
keywords = ['patch']
message_count = 13.0
messages = ['332342', '332347', '332350', '332353', '332354', '332356', '332359', '332361', '333162', '333167', '333169', '380696', '380705']
nosy_count = 8.0
nosy_names = ['gregory.p.smith', 'mark.dickinson', 'vstinner', 'eric.smith', 'ned.deily', 'serhiy.storchaka', 'miss-islington', 'xtreak']
pr_nums = ['11288', '11458', '23231']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue35560'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[tirkarthi]

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 bpo-33954 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 bpo-33954 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

[tirkarthi]

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?

cpython/Python/formatter_unicode.c

Line 529 in 59423e3

/* min_width can go negative, that's okay. format->width == -1 means

/* 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;

[serhiy-storchaka]

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.

[tirkarthi]

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 ?

[serhiy-storchaka]

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.

[tirkarthi]

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 #11288 . For some reason it's not linked to the issue.

[serhiy-storchaka]

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().

[tirkarthi]

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.

[vstinner]

New changeset 3f7983a by Victor Stinner (Xtreak) in branch 'master':
bpo-35560: Remove assertion from format(float, "n") (GH-11288)
3f7983a

[miss-islington]

New changeset 9a413fa by Miss Islington (bot) in branch '3.7':
bpo-35560: Remove assertion from format(float, "n") (GH-11288)
9a413fa

[vstinner]

Thanks Karthikeyan Singaravelan for the bug report and the fix!