This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: SystemError if invalid arguments passed to range() and step=-1
Type: behavior Stage: patch review
Components: Interpreter Core Versions: Python 3.0, Python 3.1
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, georg.brandl, laszlo
Priority: normal Keywords: needs review, patch

Created on 2008-12-04 21:20 by laszlo, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
range.diff laszlo, 2008-12-05 00:33 Updated patch to properly check arguments of range()
test_range.patch amaury.forgeotdarc, 2008-12-05 01:23
Messages (8)
msg76928 - (view) Author: Laszlo (laszlo) Date: 2008-12-04 21:20
>>> range(1.0, 0, 1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'float' object cannot be interpreted as an integer

>>> range(1.0, 0, -1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
SystemError: NULL result without error in PyObject_Call

The error here is that range() does not accept float arguments. However
in the second example the step argument is -1. Since -1 is also used to
indicate a integer overflow, when processing the step argument, it is
assumed that (step == -1 && PyErr_Occurred()) means that an overflow
occured.

However in this particular case, step is supposed to be -1, and the
error is a TypeError from the previous argument which is a float. The
error is then cleared and step is rounded to fit inside an integer.

        start = PyNumber_Index(start);
        stop = PyNumber_Index(stop);
        step = validate_step(step);
        if (!start || !stop || !step)
            goto Fail;

Now in the above code start is NULL, and the if statement checks for
this, and goes to Fail which return NULL. But no error condition is set
(it was cleared before) and NULL raises a SystemError.

My patch changes three things:
* In validate_step(): remove unnecessary 'step = PyNumber_Index(step)',
because this PyNumber_Index conversion is already done in the next line
by PyNumber_AsSsize_t().
* In validate_step(): don't clear the error is the result is -1. The
overflow error is already cleared by PyNumber_AsSsize_t(), and any other
errors should remain.
* In range_new(): check for NULL values before calling validate_step(),
because unlike for the other arguments where we call PyNumber_Index(),
calling validate_step() may clear the previous error.
msg76947 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-12-04 23:58
The analysis is good, but there are two problems with your patch:

- it crashes in debug mode, because a Py_INCREF(step) is missing in 
validate_step() (A comment above says: "Always returns a new reference")
- it does not work with 

>>> class Index:
...  def __index__(self):
...     return 42
...
>>> list(range(0, 100, Index()))
TypeError: unsupported operand type(s) for //: 'int' and 'Index'

In short: PyNumber_Index is not an unnecessary call!
But this does not invalidate the other parts of the patch. Would you try again?
msg76954 - (view) Author: Laszlo (laszlo) Date: 2008-12-05 00:33
Oops, sorry I didn't realize validate_step() is supposed to validate the
input AND return the converted value. Thanks for the feedback. 

This new patch should work fine. It retains only the two final points of
the previous patch; it doesn't clear the error, and it checks for other
NULL values before calling validate_step().
msg76960 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-12-05 01:23
The new patch is good for me.
I added unit tests, someone should review.
msg77005 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2008-12-05 11:42
Why is the overflow handling changed at all?
msg77011 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-12-05 13:03
It's right that the overflow handling path is never never taken:

If "step = PyNumber_Index(step)" succeeds, 'step' is (a subclass of) an
int, and PyNumber_AsSsize_t cannot fail, not even on low memory
conditions, and large figures are clipped.

But if it were to fail (in some future implementation), the present code
would clear an eventual exception.
Maybe this part could be applied only to 3.1.
msg77023 - (view) Author: Laszlo (laszlo) Date: 2008-12-05 14:54
It is changed from PyErr_Clear() to Py_CLEAR(step) because in case
someone else makes the same mistake of not checking for an error before
calling validate_step() we do not want to hide the error.

It is true that if used properly this part will not get executed, but if
there is another similar programming error like the patch fixes in
range_new() it will at least let the programmer see the right traceback
in the terminal.
msg89837 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2009-06-29 13:50
Seems to be fixed in current 3k head.
History
Date User Action Args
2022-04-11 14:56:42adminsetgithub: 48786
2009-06-29 13:50:49georg.brandlsetstatus: open -> closed
resolution: out of date
messages: + msg89837
2008-12-05 14:54:40laszlosetmessages: + msg77023
2008-12-05 13:03:09amaury.forgeotdarcsetmessages: + msg77011
2008-12-05 11:42:15georg.brandlsetnosy: + georg.brandl
messages: + msg77005
2008-12-05 01:23:39amaury.forgeotdarcsetkeywords: + needs review
files: + test_range.patch
messages: + msg76960
stage: patch review
2008-12-05 00:33:43laszlosetfiles: - range.diff
2008-12-05 00:33:37laszlosetfiles: + range.diff
messages: + msg76954
2008-12-04 23:58:12amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg76947
2008-12-04 21:20:36laszlocreate