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

Created on 2017-03-17 19:39 by serhiy.storchaka, last changed 2017-03-24 21:38 by serhiy.storchaka.

Pull Requests
URL Status Linked Edit
PR 700 open serhiy.storchaka, 2017-03-17 19:47
Messages (6)
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().
Date User Action Args
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