[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Tal Einat]

Tal Einat  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[STINNER Victor]

STINNER Victor  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Tal Einat]

Tal Einat  added the comment:

Thanks for all of your work on this Oren!

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[STINNER Victor]

STINNER Victor  added the comment:

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

--
nosy: +vstinner
versions: +Python 3.8 -Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Tal Einat]

Change by Tal Einat :


--
pull_requests: +9368

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Tal Einat]

Tal Einat  added the comment:

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

--
nosy: +taleinat

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Oren Milman]

Change by Oren Milman :


--
pull_requests: +3807

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Segev Finer]

Change by Segev Finer :


--
pull_requests: +3805

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Igor]

Igor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Oren Milman]

Oren Milman added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Igor]

Igor added the comment:

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 )

--
nosy: +i3v

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Oren Milman]

Oren Milman added the comment:

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

--
Added file: http://bugs.python.org/file46735/patchDraft2.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Opened issue29845 for _testcapi issues.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Oren Milman]

Oren Milman added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I suggest just remove the test with LONG_MAX.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Oren Milman]

Oren Milman added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--
nosy: +amaury.forgeotdarc, belopolsky, meador.inge
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Oren Milman]

Oren Milman added the comment:

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.

--
keywords: +patch
Added file: http://bugs.python.org/file46732/patchDraft1.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29843] errors raised by ctypes.Array for invalid _length_ attribute

[Oren Milman]

New submission from Oren Milman:

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?

--
components: ctypes
messages: 289800
nosy: Oren Milman, serhiy.storchaka
priority: normal
severity: normal
status: open
title: errors raised by ctypes.Array for invalid _length_ attribute
type: behavior
versions: Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com