Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

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
assert(overflow < 0 || length < 0);
PyErr_SetString(PyExc_ValueError,
"The '_length_' attribute must be non-negative");
}
Py_DECREF(length_attr);
Copy link
Member

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

@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

@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

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

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

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

+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

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