errors raised by ctypes.Array for invalid _length_ attribute

[orenmn]

BPO	29843
Nosy	@amauryfa, @abalkin, @vstinner, @taleinat, @meadori, @serhiy-storchaka, @orenmn
PRs	bpo-16865: Support arrays >=2GB in ctypes #3006 bpo-29843: Raise ValueError instead of OverflowError in case of a negative _length_ in a ctypes.Array subclass #3822 bpo-29843: raise AttributeError if given negative _length_ #10029
Files	patchDraft1.diff: patch draft 1 diff patchDraft2.diff: patch draft 2 diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-10-22.15:35:28.688>
created_at = <Date 2017-03-18.09:56:32.657>
labels = ['ctypes', 'type-bug', '3.8']
title = 'errors raised by ctypes.Array for invalid _length_ attribute'
updated_at = <Date 2018-10-22.19:22:18.555>
user = 'https://github.com/orenmn'

bugs.python.org fields:

activity = <Date 2018-10-22.19:22:18.555>
actor = 'taleinat'
assignee = 'none'
closed = True
closed_date = <Date 2018-10-22.15:35:28.688>
closer = 'taleinat'
components = ['ctypes']
creation = <Date 2017-03-18.09:56:32.657>
creator = 'Oren Milman'
dependencies = []
files = ['46732', '46735']
hgrepos = []
issue_num = 29843
keywords = ['patch']
message_count = 18.0
messages = ['289800', '289802', '289804', '289805', '289807', '289808', '289809', '289813', '289829', '300453', '300573', '300590', '328216', '328253', '328254', '328261', '328262', '328270']
nosy_count = 8.0
nosy_names = ['amaury.forgeotdarc', 'belopolsky', 'vstinner', 'taleinat', 'meador.inge', 'serhiy.storchaka', 'Oren Milman', 'i3v']
pr_nums = ['3006', '3822', '10029']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue29843'
versions = ['Python 3.8']

[orenmn]

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 bpo-29833 (this is a sub-issue of bpo-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?

[orenmn]

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.

[serhiy-storchaka]

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.

[orenmn]

"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?

[serhiy-storchaka]

I suggest just remove the test with LONG_MAX.

[orenmn]

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.

[serhiy-storchaka]

I'm working on this. Right now I'm searching other tests that use _testcapi without guards.

[serhiy-storchaka]

Opened bpo-29845 for _testcapi issues.

[orenmn]

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 bpo-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...

[i3v]

Oren,

won't the "too large _length_" case vanish, if #3006 would be accepted ?

( http://bugs.python.org/issue16865 )

[orenmn]

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

[i3v]

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).

[taleinat]

Should this be back-ported to 3.7 and 3.6? 2.7?

[vstinner]

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.

[serhiy-storchaka]

This change is big enough now. I think it is better to not backport it.

[taleinat]

Thanks for all of your work on this Oren!

[vstinner]

( #10029 has been merged, but GitHub webhooks are paused and so no notification has been sent to this bug yet. )

[taleinat]

New changeset 2447773 by Tal Einat in branch 'master':
bpo-29843: raise AttributeError if given negative _length_ (GH-10029)
2447773