Guido van Rossum wrote:

...

The email below is a serious bug report. A quick analysis shows that UserString.count() calls the count() method on a string object, which calls PyArg_ParseTuple() with the format string "O|ii". The 'i' format code truncates integers. It probably should raise an overflow exception instead. But that would still cause the test to fail -- just in a different way (more explicit). Then the string methods should be fixed to use long ints instead -- and then something else would probably break...

All uses in stringobject.c and unicodeobject.c use INT_MAX together with integers, so there's no problem on that side of the fence ;-)

Since strings and Unicode objects use integers to describe the length of the object (as well as most if not all other builtin sequence types), the correct default value should thus be something like sys.maxlen which then gets set to INT_MAX.

I'd suggest adding sys.maxlen and the modifying UserString.py, re.py and sre_parse.py accordingly.

Hm, I'm not so sure. It would be much better if passing sys.maxint would just WORK... Since that's what people have been doing so far.

--Guido van Rossum (home page: http://www.python.org/~guido/)

Trent Mick wrote:

...

...

Guido van Rossum wrote:

...

The email below is a serious bug report. A quick analysis shows that UserString.count() calls the count() method on a string object, which calls PyArg_ParseTuple() with the format string "O|ii". The 'i' format code truncates integers. It probably should raise an overflow exception instead. But that would still cause the test to fail -- just in a different way (more explicit). Then the string methods should be fixed to use long ints instead -- and then something else would probably break...

MAL wrote:

...

...

All uses in stringobject.c and unicodeobject.c use INT_MAX together with integers, so there's no problem on that side of the fence ;-)

Since strings and Unicode objects use integers to describe the length of the object (as well as most if not all other builtin sequence types), the correct default value should thus be something like sys.maxlen which then gets set to INT_MAX.

I'd suggest adding sys.maxlen and the modifying UserString.py, re.py and sre_parse.py accordingly.

Guido wrote:

...

Hm, I'm not so sure. It would be much better if passing sys.maxint would just WORK... Since that's what people have been doing so far.

Possible solutions (I give 4 of them): [...]

Here is another one... I don't really like it because I think that silent truncations are a bad idea, but to make things "just work it would help:

* Change PyArg_ParseTuple() to truncate the range(INT_MAX+1, LONG_MAX+1) to INT_MAX and the same for negative numbers when passing a Python integer to a "i" marked variable. This would map range(INT_MAX+1, LONG_MAX+1) to INT_MAX and thus sys.maxint would turn out as INT_MAX in all those cases where "i" is used as parser marker. Dito for negative values.

With this truncation passing sys.maxint as default argument for length parameters would "just work" :-).

The more radical alternative would be changing the Python object length fields to long -- I don't think this is practical though (and probably not really needed unless you intend to work with 3GB strings ;).

[Guido]

The email below is a serious bug report. A quick analysis shows that UserString.count() calls the count() method on a string object, which calls PyArg_ParseTuple() with the format string "O|ii". The 'i' format code truncates integers.

For people unfamiliar w/ the details, let's be explicit: the "i" code implicitly converts a Python int (== a C long) to a C int (which has no visible Python counterpart). Overflow is not detected, so this is broken on the face of it.

It probably should raise an overflow exception instead.

Definitely.

But that would still cause the test to fail -- just in a different way (more explicit). Then the string methods should be fixed to use long ints instead -- and then something else would probably break...

Yup. Seems inevitable.

[MAL]

Since strings and Unicode objects use integers to describe the length of the object (as well as most if not all other builtin sequence types), the correct default value should thus be something like sys.maxlen which then gets set to INT_MAX.

I'd suggest adding sys.maxlen and the modifying UserString.py, re.py and sre_parse.py accordingly.

I understand this, but hate it. I'd rather get rid of the user-visible distinction between the two int types already there, not add yet a third artificial int limit.

[Guido]

Hm, I'm not so sure. It would be much better if passing sys.maxint would just WORK... Since that's what people have been doing so far.

[Trent Mick]

Possible solutions (I give 4 of them):

1. The 'i' format code could raise an overflow exception and the

PyArg_ParseTuple() call in string_count() could catch it and truncate to INT_MAX (reasoning that any overflow of the end position of a string can be bound to INT_MAX because that is the limit for any string in Python).

There's stronger reason than that: string_count's "start" and "end" arguments are documented as "interpreted as in slice notation", and slice notation with out-of-range indices is well defined in all cases:

The semantics for a simple slicing are as follows. The primary must evaluate to a sequence object. The lower and upper bound expressions, if present, must evaluate to plain integers; defaults are zero and the sequence's length, respectively. If either bound is negative, the sequence's length is added to it. The slicing now selects all items with index k such that i <= k < j where i and j are the specified lower and upper bounds. This may be an empty sequence. It is not an error if i or j lie outside the range of valid indexes (such items don't exist so they aren't selected).

(From the Ref Man's section "Slicings") That is, what string_count should do is perfectly clear already (or will be, when you read that two more times <wink>). Note that you need to distinguish between positive and negative overflow, though!

Pros:

• This "would just WORK" for usage of sys.maxint.

Cons:

• This overflow exception catching should then reasonably be

propagated to other similar functions (like string.endswith(), etc).

Absolutely, but they *all* follow from what "sequence slicing* is *always* supposed to do in case of out-of-bounds indices.

• We have to assume that the exception raised in the

PyArg_ParseTuple(args, "O|ii:count", &subobj, &i, &last) call is for the second integer (i.e. 'last'). This is subtle and ugly.

Luckily <wink>, it's not that simple: exactly the same treatment needs to be given to both the optional "start" and "end" arguments, and in every function that accepts optional slice indices. So you write one utility function to deal with all that, called in case PyArg_ParseTuple raises an overflow error.

Pro or Con:

• Do we want to start raising overflow exceptions for other conversion

formats (i.e. 'b' and 'h' and 'l', the latter *can* overflow on Win64 where sizeof(long) < size(void*))? I think this is a good idea in principle but may break code (even if it *does* identify bugs in that code).

The code this would break is already broken <0.1 wink>.

2. Just change the definitions of the UserString methods to pass

a variable length argument list instead of default value parameters. For example change UserString.count() from:

def count(self, sub, start=0, end=sys.maxint):
    return self.data.count(sub, start, end)

to:

def count(self, *args)):
    return self.data.count(*args)

The result is that the default value for 'end' is now set by string_count() rather than by the UserString implementation: ...

This doesn't solve anything -- users can (& do) pass sys.maxint explicitly. That's part of what Guido means by "since that's what people have been doing so far".

... Cons:

• Does not fix the general problem of the (common?) usage of sys.maxint to

mean INT_MAX rather than the actual LONG_MAX (this matters on 64-bit Unices).

Anyone using sys.maxint to mean INT_MAX is fatally confused; passing sys.maxint as a slice index is not an instance of that confusion, it's just relying on the documented behavior of out-of-bounds slice indices.

3. As MAL suggested: add something like sys.maxlen (set to INT_MAX) with

breaks the logical difference with sys.maxint (set to LONG_MAX): ...

I hate this (see above).

... 4. Add something like sys.maxlen, but set it to SIZET_MAX (c.f. ANSI size_t type). It is probably not a biggie, but Python currently makes the assumption that string never exceed INT_MAX in length.

It's not an assumption, it's an explicit part of the design: PyObject_VAR_HEAD declares ob_size to be an int. This leads to strain for sure, partly because the *natural* limit on sizes is derived from malloc (which indeed takes neither int nor long, but size_t), and partly because Python exposes no corresponding integer type. I vaguely recall that this was deliberate, with the *intent* being to save space in object headers on the upcoming 128-bit KSR machines <wink>.

While this assumption is not likely to be proven false it technically could be on 64-bit systems.

Well, Guido once said he would take away Python's recursion overflow checks just as soon as machines came with infinite memory <wink> -- 2Gb is a reasonable limit for string length, and especially if it's a tradeoff against increasing the header size for all string objects (it's almost certainly more important to cater to oodles of small strings on smaller machines than to one or two gigantic strings on huge machines).

As well, when you start compiling on Win64 (where sizeof(int) == sizeof(long) < sizeof(size_t)) then you are going to be annoyed by hundreds of warnings about implicit casts from size_t (64-bits) to int (32-bits) for every strlen, str*, fwrite, and sizeof call that you make.

Every place the code implicitly downcasts from size_t to int is plainly broken today, so we *should* get warnings. Python has been sloppy about this! In large part it's because Python was written before ANSI C, and size_t simply wasn't supported at the time. But as with all software, people rarely go back to clean up; it's overdue (just be thankful you're not working on the Perl source <0.9 wink>).

Pros:

• IMHO logically more correct.
• Might clean up some subtle bugs.
• Cleans up annoying and disconcerting warnings.
• Will probably mean less pain down the road as 64-bit systems

(esp. Win64) become more prevalent.

Cons:

• Lot of coding changes.
• As Guido said: "and then something else would probably break".

(Though, on currently 32-bits system, there should be no effective change). Only 64-bit systems should be affected and, I would hope, the effect would be a clean up.

I support this as a long-term solution, perhaps for P3K. Note that ob_refcnt should also be declared size_t (no overflow-checking is done on refcounts today; the theory is that a refcount can't possibly get bigger than the total # of pointers in the system, and so if you declare ob_refcnt to be large enough to hold that, refcount overflow is impossible; but, in theory, this argument has been broken on every machine where sizeof(int) < sizeof(void*)).

I apologize for not being succinct.

Humbug -- it was a wonderfully concise posting, Trent! The issues are simply messy.

Note that I am volunteering here. Opinions and guidance please.

Alas, the first four letters in "guidance" spell out four-fifths of the only one able to give you that.

opinions-are-fun-but-don't-count<wink>-ly y'rs - tim

I posted a couple of patches a couple of days ago to correct the string methods implementing slice-like optional parameters (count, find, index, rfind, rindex) to properly clamp slice index values to the proper range (any PyInt or PyLong value is acceptible now). In fact the slice_index() function that was being used in ceval.c was reused (renamed to _PyEval_SliceIndex).

As well, the other patch changes PyArg_ParseTuple's 'b', 'h', and 'i' formatters to raise an OverflowError if they overflow.

Trent

p.s. I thought I would whine here for some more attention. Who needs that Unicode stuff anyway. ;-)