classification
Title: errors raised by ctypes.Array for invalid _length_ attribute
Type: behavior Stage: patch review
Components: ctypes Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Oren Milman, amaury.forgeotdarc, belopolsky, i3v, meador.inge, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2017-03-18 09:56 by Oren Milman, last changed 2017-09-29 14:26 by Oren Milman.

Files
File name Uploaded Description Edit
patchDraft1.diff Oren Milman, 2017-03-18 10:14 patch draft 1 diff
patchDraft2.diff Oren Milman, 2017-03-18 20:59 patch draft 2 diff
Pull Requests
URL Status Linked Edit
PR 3006 Segev Finer, 2017-09-29 13:14
PR 3822 open Oren Milman, 2017-09-29 14:26
Messages (12)
msg289800 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-03-18 09:56
With regard to ctypes.Array:

currently:
>>> from ctypes import *
>>> class T(Array):
...     _type_ = c_int
...     _length_ = -1
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: array too large
>>> class T(Array):
...     _type_ = c_int
...     _length_ = -1 << 1000
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: The '_length_' attribute is too large


Obviously, here the _length_ attribute is too small, not too large.
Thus, the error messages should be changed to be more accurate (optimally, for
any negative _length_, the error message should be the same).

Moreover, in accordance with #29833 (this is a sub-issue of #29833), ValueError
should be raised for any negative _length_ attribute (instead of
OverflowError).


Also, Note that currently, in case _length_ == 0, no error is raised. ISTM that
a ctypes Array of length 0 is useless, so maybe we should raise a ValueError in
this case too?
msg289802 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-03-18 10:14
A patch draft (which doesn't change anything about the case of _length_ == 0)
is attached.
(I am not opening a PR, because I am not sure the behavior change would be
accepted.)

I ran the test module on my Windows 10, and it seems the patch doesn't break
anything.
msg289804 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-18 10:50
LGTM, but I prefer `overflow > 0` over `overflow == 1`.

If use _testcapi the tests should be decorated with cpython_only. But I think that it is better to not use it. Limiting _length_ to C long (rather than size_t) is an implementation detail. The test with _length_ = 1 << 1000 should be enough.
msg289805 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-03-18 11:01
"If use _testcapi the tests should be decorated with cpython_only."

at first, I thought so too, but then I searched for 'cpython_only' in
Lib/ctypes/test, and found nothing. then I searched for '_testcapi' there,
and found a top level 'import _testcapi' in
Lib/ctypes/test/test_structures.py, and a test using _testcapi.INT_MAX.

so I assumed that test_ctypes itself is cpython_only.

should test_structures.py be changed, then?
msg289807 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-18 11:07
I suggest just remove the test with LONG_MAX.
msg289808 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-03-18 11:19
yes, you are right, of course.


but regardless of this issue, shouldn't test_structures.py be changed
(in a seperate issue)?
ISTM it has a cpython-specific test not decorated in @cpython_only.
msg289809 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-18 11:22
I'm working on this. Right now I'm searching other tests that use _testcapi without guards.
msg289813 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-18 13:55
Opened issue29845 for _testcapi issues.
msg289829 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-03-18 20:59
here is the patch updated according to your suggestions, Serhiy.


however, I wonder about the case of a too large _length_.
shouldn't we raise a MemoryError in such a case, in accordance with #29833?

BTW, while inspecting code related to a too large _length_, I came across this
(in PyCArrayType_new):
    if (length * itemsize < 0) {
        PyErr_SetString(PyExc_OverflowError,
                        "array too large");
        goto error;
    }
I am not sure, but isn't this check unsafe? (e.g. if length == 2 ** 30, and
itemsize == 4, couldn't the product be 0 on some platforms?)
but maybe the code before this check makes more checks. I didn't make a
thorough inspection...
msg300453 - (view) Author: Igor (i3v) Date: 2017-08-17 18:27
Oren, 

won't the "too large _length_" case vanish, if https://github.com/python/cpython/pull/3006 would be accepted ?

( http://bugs.python.org/issue16865 )
msg300573 - (view) Author: Oren Milman (Oren Milman) * Date: 2017-08-19 08:39
I am not sure I understood your question, Igor.

I compiled with https://github.com/python/cpython/pull/3006, and got:
    class T(ctypes.Array):
        _type_ = ctypes.c_int
        _length_ = 2 ** 1000
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    OverflowError: Python int too large to convert to C ssize_t

and also:
    class T(ctypes.Array):
        _type_ = ctypes.c_int
        _length_ = -1
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    OverflowError: array too large
msg300590 - (view) Author: Igor (i3v) Date: 2017-08-19 23:17
Oren,

1) I might be completely wrong, but, personally, I think about OverflowError vs ValueError difference like this: if the value couldn't be handled because method's logic cannot handle it - it's a ValueError; if it could not be handled because of a low-level platform-dependent limitation - it's an OverflowError. Before that PR, the _length_ maximum value was hard-coded in the method itself, thus one might say that it was "a part of logic". With this PR, you just need a system with a large enough size_t. 
(May be, after a thousand years, it would even handle 2**1000. But negative values would be still logically incorrect. Thus, I'm only talking about "too large" case.)

2) It would be much more difficult to run into this limitation in a daily practice (e.g. by passing a very long string).
History
Date User Action Args
2017-09-29 14:26:33Oren Milmansetpull_requests: + pull_request3807
2017-09-29 13:14:04Segev Finersetpull_requests: + pull_request3805
2017-08-19 23:17:48i3vsetmessages: + msg300590
2017-08-19 08:39:35Oren Milmansetmessages: + msg300573
2017-08-17 18:27:23i3vsetnosy: + i3v
messages: + msg300453
2017-03-18 20:59:08Oren Milmansetfiles: + patchDraft2.diff

messages: + msg289829
2017-03-18 13:55:15serhiy.storchakasetmessages: + msg289813
2017-03-18 11:22:04serhiy.storchakasetmessages: + msg289809
2017-03-18 11:19:23Oren Milmansetmessages: + msg289808
2017-03-18 11:07:00serhiy.storchakasetmessages: + msg289807
2017-03-18 11:01:00Oren Milmansetmessages: + msg289805
2017-03-18 10:50:10serhiy.storchakasetnosy: + amaury.forgeotdarc, belopolsky, meador.inge

messages: + msg289804
stage: patch review
2017-03-18 10:14:45Oren Milmansetfiles: + patchDraft1.diff
keywords: + patch
messages: + msg289802
2017-03-18 09:56:32Oren Milmancreate