StaticTuple_New handles size < 0 and size > 255 differently

Bug #457979 reported by Matt Nordhoff on 2009-10-22
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Bazaar
Wishlist
Matt Nordhoff

Bug Description

Lookie:

    if (size < 0) {
        PyErr_BadInternalCall();
        return NULL;
    }

    if (size < 0 || size > 255) {
        /* Too big or too small */
        PyErr_SetString(PyExc_ValueError, "StaticTuple(...)"
            " takes from 0 to 255 items");
        return NULL;
    }

Not only is this redundant, but PyErr_BadInternalCall() throws a TypeError, meaning two different exception classes are used.

ISTM StaticTuple_New should throw a ValueError, since you do StaticTuple_New(length), and StaticTuple_new_constructor should throw a TypeError, since you do StaticTuple(foo, bar, baz) (currently it lets StaticTuple_New handle throw the error).

FYI, the Python version of StaticTuple throws a ValueError when it's either <0 or >255. IMO, that should be a TypeError, same as StaticTuple_new_constructor.

Related branches

description: updated
Download full text (3.5 KiB)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Matt Nordhoff wrote:
> Public bug reported:
>
> Lookie:
>
> if (size < 0) {
> PyErr_BadInternalCall();
> return NULL;
> }
>
> if (size < 0 || size > 255) {
> /* Too big or too small */
> PyErr_SetString(PyExc_ValueError, "StaticTuple(...)"
> " takes from 0 to 255 items");
> return NULL;
> }
>
> Not only is this redundant, but PyErr_BadInternalCall() throws a
> TypeError, meaning two different exception classes are used.
>
> ISTM StaticTuple_New should throw a ValueError, since you do
> StaticTuple_New(length), and StaticTuple_new_constructor should throw a
> TypeError, since you do StaticTuple(foo, bar, baz) (currently it lets
> StaticTuple_New handle throw the error).
>
> FYI, the Python version of StaticTuple throws a ValueError when it's
> either <0 or >255. IMO, that should be a TypeError, same as
> StaticTuple_new_constructor.
>
> ** Affects: bzr
> Importance: Undecided
> Status: New
>
> ** Description changed:
>
> Lookie:
>
> - if (size < 0) {
> - PyErr_BadInternalCall();
> - return NULL;
> - }
> + if (size < 0) {
> + PyErr_BadInternalCall();
> + return NULL;
> + }
>
> - if (size < 0 || size > 255) {
> - /* Too big or too small */
> - PyErr_SetString(PyExc_ValueError, "StaticTuple(...)"
> - " takes from 0 to 255 items");
> - return NULL;
> - }
> + if (size < 0 || size > 255) {
> + /* Too big or too small */
> + PyErr_SetString(PyExc_ValueError, "StaticTuple(...)"
> + " takes from 0 to 255 items");
> + return NULL;
> + }
>
> Not only is this redundant, but PyErr_BadInternalCall() throws a
> TypeError, meaning two different exception classes are used.

Actually, PyErr_BadInternalCall raises a "SystemError":
void
_PyErr_BadInternalCall(char *filename, int lineno)
{
 PyErr_Format(PyExc_SystemError,

...

My specific point was that passing size < 0 is a sign that something is
very wrong. Which is different from just passing a value that is too large.

The redundant "<0" is probably because I used to check for size < 1, and
just didn't remove that check when I started allowing size 0 StaticTuples.

>
> ISTM StaticTuple_New should throw a ValueError, since you do
> StaticTuple_New(length), and StaticTuple_new_constructor should throw a
> - TypeError, since you do StaticTuple(foo, bar, baz).
> + TypeError, since you do StaticTuple(foo, bar, baz) (currently it lets
> + StaticTuple_New handle throw the error).
>
> FYI, the Python version of StaticTuple throws a ValueError when it's
> either <0 or >255. IMO, that should be a TypeError, same as
> StaticTuple_new_constructor.
>

I'm happy to have them the same. Regardless of what we make this, it
isn't something you should ever be catching, so the specific exception
isn't particularly meaningful.

It is arguable that "TypeError" should be reserved for when the *type*
of the argument passed is invalid. (Such as passing a dict rather than a
string/unicode/etc.)

Everything else would be considered a ...

Read more...

Download full text (3.9 KiB)

John A Meinel wrote:
> Matt Nordhoff wrote:
>> Public bug reported:
>
>> Lookie:
>
>> if (size < 0) {
>> PyErr_BadInternalCall();
>> return NULL;
>> }
>
>> if (size < 0 || size > 255) {
>> /* Too big or too small */
>> PyErr_SetString(PyExc_ValueError, "StaticTuple(...)"
>> " takes from 0 to 255 items");
>> return NULL;
>> }
>
>> Not only is this redundant, but PyErr_BadInternalCall() throws a
>> TypeError, meaning two different exception classes are used.
>
>> ISTM StaticTuple_New should throw a ValueError, since you do
>> StaticTuple_New(length), and StaticTuple_new_constructor should throw a
>> TypeError, since you do StaticTuple(foo, bar, baz) (currently it lets
>> StaticTuple_New handle throw the error).
>
>> FYI, the Python version of StaticTuple throws a ValueError when it's
>> either <0 or >255. IMO, that should be a TypeError, same as
>> StaticTuple_new_constructor.
>
>> ** Affects: bzr
>> Importance: Undecided
>> Status: New
>
>> ** Description changed:
>
>> Lookie:
>
>> - if (size < 0) {
>> - PyErr_BadInternalCall();
>> - return NULL;
>> - }
>> + if (size < 0) {
>> + PyErr_BadInternalCall();
>> + return NULL;
>> + }
>
>> - if (size < 0 || size > 255) {
>> - /* Too big or too small */
>> - PyErr_SetString(PyExc_ValueError, "StaticTuple(...)"
>> - " takes from 0 to 255 items");
>> - return NULL;
>> - }
>> + if (size < 0 || size > 255) {
>> + /* Too big or too small */
>> + PyErr_SetString(PyExc_ValueError, "StaticTuple(...)"
>> + " takes from 0 to 255 items");
>> + return NULL;
>> + }
>
>> Not only is this redundant, but PyErr_BadInternalCall() throws a
>> TypeError, meaning two different exception classes are used.
>
> Actually, PyErr_BadInternalCall raises a "SystemError":
> void
> _PyErr_BadInternalCall(char *filename, int lineno)
> {
> PyErr_Format(PyExc_SystemError,
>
> ...

Oh, huh. Thanks for nothing,
<http://docs.python.org/c-api/exceptions.html#PyErr_BadInternalCall>.

> My specific point was that passing size < 0 is a sign that something is
> very wrong. Which is different from just passing a value that is too large.

Hm, that makes sense.

> The redundant "<0" is probably because I used to check for size < 1, and
> just didn't remove that check when I started allowing size 0 StaticTuples.

Oh, okay.

>> ISTM StaticTuple_New should throw a ValueError, since you do
>> StaticTuple_New(length), and StaticTuple_new_constructor should throw a
>> - TypeError, since you do StaticTuple(foo, bar, baz).
>> + TypeError, since you do StaticTuple(foo, bar, baz) (currently it lets
>> + StaticTuple_New handle throw the error).
>
>> FYI, the Python version of StaticTuple throws a ValueError when it's
>> either <0 or >255. IMO, that should be a TypeError, same as
>> StaticTuple_new_constructor.
>
>
> I'm happy to have them the same. Regardless of what we make this, it
> isn't something you should ever be catching, so the specific exception
> isn't particularly meaningful.
>
> It is arguable t...

Read more...

Martin Pool (mbp) on 2010-02-17
Changed in bzr:
importance: Undecided → Wishlist
status: New → In Progress
Changed in bzr:
assignee: nobody → Matt Nordhoff (mnordhoff)
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers