Title: Check that sq_length and mq_length return non-negative result
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.7
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: rhettinger, serhiy.storchaka, terry.reedy, vstinner
Priority: normal Keywords:

Created on 2017-03-17 19:39 by serhiy.storchaka, last changed 2017-04-16 07:07 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 700 merged serhiy.storchaka, 2017-03-17 19:47
Messages (7)
msg289777 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-17 19:39
Following PR adds several asserts for checking that sq_length and mq_length either return non-negative result or raise an exception.

One assert already was in PySequence_GetItem().
msg290107 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-03-24 19:18
The changes appear consistent within themselves (ie, no apparent typos), and the one for builtin len appears to fix a bug that would allow negative length.  Can tests be written in Python that fail without the patch?   Or would mis-behaving classes have to be written in C?
msg290110 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-24 19:38
Mis-behaving classes have to be written in C. If the __len__ method in Python class returns a negative value, ValueError is raised in slot_sq_length().

PySequence_GetItem() already contains an assertion that sq_length returns negative value only when an exception is set. The patch just extends this to other cases of calling sq_length and mq_length.
msg290111 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-24 19:44
The alternate approach is raising SystemError rather than crashing in debug build on assertion. Since this check is used in exceptional case (when a raised exception is expected) it doesn't slow down working code.
msg290145 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-03-24 21:27
Please don't add more code to critical paths.  

There is no demonstrated need for an additional check here.  The current code has worked well for millions of users for many years.

Also, there are too many checks in common paths and some of them are redundant.  We need less of this, not more.
msg290148 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-24 21:38
assert() is no-op in release build and that are not critical parts.

Actually the patch decreases the number of checks in release build. Runtime check PyErr_Occurred() is moved into assert() in builtin_len().
msg291737 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-04-16 06:21
New changeset 813f943c592cf225871b99cffc99304c8cbbee40 by Serhiy Storchaka in branch 'master':
bpo-29838: Add asserts for checking results of sq_length and mq_length slots. (#700)
Date User Action Args
2017-04-16 07:07:23serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-04-16 06:21:45serhiy.storchakasetmessages: + msg291737
2017-04-07 15:53:08serhiy.storchakasetassignee: serhiy.storchaka
2017-03-24 21:38:38serhiy.storchakasetmessages: + msg290148
2017-03-24 21:27:05rhettingersetnosy: + rhettinger
messages: + msg290145
2017-03-24 19:44:17serhiy.storchakasetmessages: + msg290111
2017-03-24 19:38:47serhiy.storchakasetmessages: + msg290110
2017-03-24 19:18:34terry.reedysetnosy: + terry.reedy
messages: + msg290107
2017-03-17 19:47:54serhiy.storchakasetpull_requests: + pull_request574
2017-03-17 19:39:52serhiy.storchakacreate