Skip to content

bpo-29843: Raise ValueError instead of OverflowError in case of a negative _length_ in a ctypes.Array subclass#3822

Closed
orenmn wants to merge 2 commits intopython:masterfrom
orenmn:bpo29843-fix-neg-_length_-handling
Closed

bpo-29843: Raise ValueError instead of OverflowError in case of a negative _length_ in a ctypes.Array subclass#3822
orenmn wants to merge 2 commits intopython:masterfrom
orenmn:bpo29843-fix-neg-_length_-handling

Conversation

@orenmn
Copy link
Contributor

@orenmn orenmn commented Sep 29, 2017 •
edited by bedevere-bot
Loading

  • in _ctypes.c: add checks to find whether _length_ is negative or too big, and raise an error accordingly.
  • in test_arrays.py: add tests to verify that the right error is raised for various values of _length_.

https://bugs.python.org/issue29843

serhiy-storchaka
serhiy-storchaka approved these changes Sep 29, 2017
View reviewed changes
Modules/_ctypes/_ctypes.c Outdated
PyErr_SetString(PyExc_ValueError,
"The '_length_' attribute must be non-negative");
}
Py_DECREF(length_attr);
Copy link
Member

@serhiy-storchaka serhiy-storchaka Sep 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be moved right after PyLong_AsLongAndOverflow().

taleinat
taleinat approved these changes Sep 6, 2018
View reviewed changes
Copy link
Contributor

@taleinat taleinat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@serhiy-storchaka, can this go in?

@vstinner
Copy link
Member

vstinner commented Oct 20, 2018

@orenmn: Can you please update or rebase this change?

@orenmn
Copy link
Contributor Author

orenmn commented Oct 20, 2018

@orenmn: Can you please update or rebase this change?

I am sorry, but I am currently very busy, and so I would be happy if someone else did that.

@taleinat taleinat force-pushed the bpo29843-fix-neg-_length_-handling branch from 3dddab3 to 8e3d4ba Compare October 21, 2018 12:37
@taleinat
Copy link
Contributor

taleinat commented Oct 21, 2018

@orenmn: Can you please update or rebase this change?

I am sorry, but I am currently very busy, and so I would be happy if someone else did that.

@vstinner, rebased.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Oct 21, 2018

It is not so easy, since length is of type Py_ssize_t.

In addition, there is a behavior difference between PyLong_AsSsize_t() and PyLong_AsLongAndOverflow(). The latter accepts not just int and its subclasses, but also obbjects implementing __int__ (e.g. float). This is not desirable.

@taleinat
Copy link
Contributor

taleinat commented Oct 21, 2018

Can we just add a Py_SIZE() check?

@taleinat
Copy link
Contributor

taleinat commented Oct 21, 2018 •
edited
Loading

Can we just add a Py_SIZE() check?

I mean _PyLong_Sign().

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Oct 21, 2018

Perhaps. Just now the solution of this issue requires different code.

@taleinat taleinat mentioned this pull request Oct 21, 2018
Merged
@vstinner
Copy link
Member

vstinner commented Oct 22, 2018

+1 to keep PyLong_AsSsize_t() but use _PyLong_Sign() to raise the ValueError (for negative number).

@taleinat: are you interested to rework this PR or maybe create a new one based on this PR?

@taleinat
Copy link
Contributor

taleinat commented Oct 22, 2018

This is superseded by PR GH-10029.

@taleinat taleinat closed this Oct 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@taleinat taleinat taleinat approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees

No one assigned

Labels

awaiting merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@orenmn @vstinner @taleinat @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot