Crash #2 (constructed overflow) in _PySequence_BytesToCharpArray()

[loewis]

http://bugs.python.org/review/15736/diff/5793/Lib/test/test_capi.py
File Lib/test/test_capi.py (right):

http://bugs.python.org/review/15736/diff/5793/Lib/test/test_capi.py#newcode93
Lib/test/test_capi.py:93: # Issue #XYZ: overflow in
_PySequence_BytesToCharpArray()
This should be #15736.

http://bugs.python.org/review/15736/diff/5793/Objects/abstract.c
File Objects/abstract.c (right):

http://bugs.python.org/review/15736/diff/5793/Objects/abstract.c#newcode2713
Objects/abstract.c:2713: if (argc > (PY_SSIZE_T_MAX-sizeof(char *)) /
sizeof(char *)) {
I think this introduces a signed/unsigned comparison. So you need to convert the
RHS operand to Py_ssize_t.

[skrah]

http://bugs.python.org/review/15736/diff/5793/Objects/abstract.c
File Objects/abstract.c (right):

http://bugs.python.org/review/15736/diff/5793/Objects/abstract.c#newcode2713
Objects/abstract.c:2713: if (argc > (PY_SSIZE_T_MAX-sizeof(char *)) /
sizeof(char *)) {
Good point. I assumed that PySequence_Size(self) never returns a negative size
(except -1 == error), based on this test case:

class Z(object):
    def __len__(self):
        return -1

_posixsubprocess.fork_exec(1,Z(),3,[1, 2],5,6,7,8,9,10,11,12,13,14,15,16,17)

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: __len__() should return >= 0


But that might be wrong. -- I think due to arithmetic conversions
(PY_SSIZE_T_MAX-sizeof(char *)) is already size_t, so how about checking for ...

    argc = PySequence_Size(self);
    if (argc < 0)
	
... and using (size_t)argc in the overflow comparison? The explicit cast should
get rid of compiler warnings.

[loewis]

http://bugs.python.org/review/15736/diff/5793/Objects/abstract.c
File Objects/abstract.c (right):

http://bugs.python.org/review/15736/diff/5793/Objects/abstract.c#newcode2713
Objects/abstract.c:2713: if (argc > (PY_SSIZE_T_MAX-sizeof(char *)) /
sizeof(char *)) {
On 2012/08/20 15:19:22, skrah wrote:
> Good point. I assumed that PySequence_Size(self) never returns a negative size
> (except -1 == error)

That's actually not my point. Instead, (I think) we are trying to get rid of
signed-unsigned comparisons, as compilers will complain about them, see e.g.
issue 10951.

I certainly agree that this specific comparison is semantically safe.

> ... and using (size_t)argc in the overflow comparison? The explicit cast
should
> get rid of compiler warnings.

That would work, as would an assert(argc>=0), as would a comment claiming that
it must be >=0. Of these, I think I'd like the assert best - PySequence_Size
is specified to return -1 on failure, and any sq_length returning some other
negative value is buggy.

[loewis]

http://bugs.python.org/review/15736/diff/5798/Objects/abstract.c
File Objects/abstract.c (right):

http://bugs.python.org/review/15736/diff/5798/Objects/abstract.c#newcode2710
Objects/abstract.c:2710: if (argc < 0)
I find this test somewhat confusing. It suggests that any negative value might
be the result, when the only "legal" negative value is -1.

[skrah]

http://bugs.python.org/review/15736/diff/5798/Objects/abstract.c
File Objects/abstract.c (right):

http://bugs.python.org/review/15736/diff/5798/Objects/abstract.c#newcode2710
Objects/abstract.c:2710: if (argc < 0)
This was meant to get an assert "for free". But I also like the explicit
assert(argc >= 0), so let's just do that instead.