| 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 "", line 1, in OverflowError: array too large >>> class T(Array): ... _type_ = c_int ... _length_ = -1 << 1000 ... Traceback (most recent call last): File "", line 1, in 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) *  |
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) * |
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) * |
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) * |
Date: 2017-03-18 13:55 |
| Opened 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 "", line 1, in 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 "", line 1, in 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). |
|
|
| msg328216 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2018-10-21 14:56 |
| Should this be back-ported to 3.7 and 3.6? 2.7? |
|
|
| msg328253 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2018-10-22 14:06 |
| > Should this be back-ported to 3.7 and 3.6? 2.7? IMHO it's a bad idea to introduce a *new* exception in stable version. I suggest to only modify the master branch. |
|
|
| msg328254 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-10-22 14:12 |
| This change is big enough now. I think it is better to not backport it. |
|
|
| msg328261 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2018-10-22 15:35 |
| Thanks for all of your work on this Oren! |
|
|
| msg328262 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2018-10-22 15:53 |
| ( https://github.com/python/cpython/pull/10029 has been merged, but GitHub webhooks are paused and so no notification has been sent to this bug yet. ) |
|
|
| msg328270 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2018-10-22 19:22 |
| New changeset 2447773573e74819e163f8963ab107bc5db123e5 by Tal Einat in branch 'master': bpo-29843: raise AttributeError if given negative _length_ (GH-10029) https://github.com/python/cpython/commit/2447773573e74819e163f8963ab107bc5db123e5 |
|
|