Remove IsBadStringPtr calls in ctypes

[zooba]

BPO	23765
Nosy	@tjguk, @zware, @eryksun, @zooba

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/zooba'
closed_at = <Date 2015-04-03.15:20:37.894>
created_at = <Date 2015-03-24.18:39:05.834>
labels = ['ctypes', 'type-feature', 'OS-windows']
title = 'Remove IsBadStringPtr calls in ctypes'
updated_at = <Date 2015-04-03.15:20:37.894>
user = 'https://github.com/zooba'

bugs.python.org fields:

activity = <Date 2015-04-03.15:20:37.894>
actor = 'steve.dower'
assignee = 'steve.dower'
closed = True
closed_date = <Date 2015-04-03.15:20:37.894>
closer = 'steve.dower'
components = ['Windows', 'ctypes']
creation = <Date 2015-03-24.18:39:05.834>
creator = 'steve.dower'
dependencies = []
files = []
hgrepos = []
issue_num = 23765
keywords = []
message_count = 9.0
messages = ['239167', '239176', '239221', '239239', '239250', '239284', '239287', '239304', '239305']
nosy_count = 5.0
nosy_names = ['tim.golden', 'python-dev', 'zach.ware', 'eryksun', 'steve.dower']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue23765'
versions = []

[zooba]

Modules/_ctypes/cfield.c has this horror in it (twice):

    /* XXX What about invalid pointers ??? */
    if (*(void **)ptr) {
#if defined(MS_WIN32) && !defined(_WIN32_WCE)
        if (IsBadStringPtrA(*(char **)ptr, -1)) {
            PyErr_Format(PyExc_ValueError,
                         "invalid string pointer %p",
                         *(char **)ptr);
            return NULL;
        }
#endif
        return PyBytes_FromStringAndSize(*(char **)ptr,
                                         strlen(*(char **)ptr));

IsBadStringPtr should generally not be used, and the -1 parameter makes it even worse. See http://blogs.msdn.com/b/oldnewthing/archive/2006/09/27/773741.aspx for details, but the main reason is that if it is actually a bad pointer, we've just deferred the crash from the obvious location to somewhere that should "never" crash.

The strlen() call has exactly the same behaviour as IsBadStringPtrA except the crash will occur here.

A better alternative would be to use the safe strlen function to limit the maximum length of strings, but since we likely can't agree on a suitable maximum we should just stop trying to handle this case at all.

[tjguk]

Unless someone comes back who remembers what the ulterior motive was: I
agree; remove the check and just the crash happen.

[python-dev]

New changeset 585e555247ac by Steve Dower in branch 'default':
Issue bpo-23765: Remove IsBadStringPtr calls in ctypes
https://hg.python.org/cpython/rev/585e555247ac

[eryksun]

c_char_p.__repr__ (defined in __init__.py) also checks IsBadStringPtrA via FFI.

Defining the repr differently on Windows is a wart, IMO. The following repr should be used on all platforms for c_char_p:

"%s(%s)" % (self.__class__.__name__, 
            c_void_p.from_buffer(self).value)

On a related note, wide-character c_wchar_p uses the default _SimpleCData.__repr__, which calls the getfunc. It should be overridden in the same manner as c_char_p because this can easily segfault Python on a non-Windows platform. Even on Windows, calling IsBadStringPtrW in Z_get can't make guarantees given multiple threads. The GIL helps, but with ctypes there's a good chance that many threads are running concurrently.

[zooba]

Thanks, I forgot to scan .py files.

[zooba]

So the repr that's there for c_char_p is currently::

"%s(%s)" % (self.__class__.__name__, cast(self, c_void_p).value)

But if I remove the override then it renders the value as b'abc'. Basically, we can have one of:

>>> from ctypes import *
>>> cast(create_string_buffer(b'abc'), c_char_p)
c_char_p(b'abc')

or

>>> cast(create_string_buffer(b'abc'), c_char_p)
c_char_p(52808208)

I prefer the former (remove c_char_p.__repr__ completely), but the latter is clearly there for some reason. Any opinions?

[eryksun]

So the repr that's there for c_char_p is currently::
"%s(%s)" % (self.__class__.__name__, cast(self, c_void_p).value)

I suggested switching to using from_buffer:

c_void_p.from_buffer(self).value 

from_buffer is more efficient. cast() is imlemented as an FFI call. There's also another approach using the buffer protocol:

    int.from_bytes(self, sys.byteorder)

I prefer the former (remove c_char_p.__repr__ completely), but
the latter is clearly there for some reason. Any opinions?

It's there for same reason I suggested overriding c_wchar_p.__repr__. Getting the repr of an object shouldn't segfault the interpreter. The existing check with IsBadStringPtrA is bogus in a multithreaded environment. The repr should just show the address. At least that won't crash.

A topic for another discussion is disabling the SEH handler because it's not used consistently anyway (e.g. to guard pointer and CData base pointer access). To compensate, the faulthandler could be enhanced. It could call the CRT's __pxcptinfoptrs (not documented, but it should be) to get the Windows EXCEPTION_POINTERS record. This would enable at least an improved error message, e.g. "Segmentation fault reading [addr]" and "Segmentation fault writing [addr]". The funny thing is the current faulthandler example uses ctypes.string_at, which, because it's implemented as an FFI call guarded by the SEH handler, just raises OSError on Windows.

[zooba]

Ah okay, I understand better what your suggestion was trying to achieve now. I agree, we should just show the address in repr - I'll make that change to both c_char_p and c_wchar_p.

[python-dev]

New changeset 339d90983e29 by Steve Dower in branch 'default':
Issue bpo-23765: Removed IsBadStringPtr calls in ctypes
https://hg.python.org/cpython/rev/339d90983e29