Passing 'None' if argtype is set to POINTER(...) doesn't always result in NULL

[robertluce]

BPO	4606
Nosy	@theller
Files	patch_ctypes_none_arg.diff: proposed patch against 2.6 patch-ctypes-none-arg-2.diff: New patch against trunk, added the missing Py_INCREF().

^{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/theller'
closed_at = <Date 2009-09-18.20:13:47.659>
created_at = <Date 2008-12-09.09:46:59.651>
labels = ['ctypes', 'type-crash']
title = "Passing 'None' if argtype is set to POINTER(...) doesn't always result in NULL"
updated_at = <Date 2011-04-11.12:49:47.597>
user = 'https://bugs.python.org/robertluce'

bugs.python.org fields:

activity = <Date 2011-04-11.12:49:47.597>
actor = 'olt'
assignee = 'theller'
closed = True
closed_date = <Date 2009-09-18.20:13:47.659>
closer = 'theller'
components = ['ctypes']
creation = <Date 2008-12-09.09:46:59.651>
creator = 'robertluce'
dependencies = []
files = ['12300', '14608']
hgrepos = []
issue_num = 4606
keywords = ['patch']
message_count = 10.0
messages = ['77397', '82371', '91082', '91103', '91104', '91112', '91144', '91145', '92841', '133518']
nosy_count = 5.0
nosy_names = ['theller', 'amcnabb', 'LambertDW', 'olt', 'robertluce']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue4606'
versions = ['Python 2.6', 'Python 3.1', 'Python 2.7', 'Python 3.2']

[robertluce]

Consider the library 'c_lib.so' consisting of a single function 'c_func'

int c_func ( double *arg0, double *arg1, double *arg2, double *arg3,
             double *arg4, double *arg5, double *arg6) {
    printf("Value of arg0 is %p\n", arg0);
    printf("Value of arg1 is %p\n", arg1);
    printf("Value of arg2 is %p\n", arg2);
    printf("Value of arg3 is %p\n", arg3);
    printf("Value of arg4 is %p\n", arg4);
    printf("Value of arg5 is %p\n", arg5);
    printf("Value of arg6 is %p\n", arg6);
    return 0;
}

and the following snippet:

from ctypes import *
c_lib = CDLL('c_lib.so')
t = POINTER(c_double)
c_lib.c_func.argtypes = [t,t,t,t,t,t,t]

def call_c_func():
    nptr = None
    c_lib.c_func(nptr, nptr, nptr, nptr, nptr, nptr, nptr)

The output I get from call_c_func() with Python 2.6 and Python 2.5 on a
64 bit Linux box is (it probably won't happen on 32 bit systems):

Value of arg0 is (nil)
Value of arg1 is (nil)
Value of arg2 is (nil)
Value of arg3 is (nil)
Value of arg4 is (nil)
Value of arg5 is (nil)
Value of arg6 is 0xa00000000

The reason appears to be that in 'PointerType_from_param' (_ctypes.c)
Py_None is converted to a Python integer of value 0. Later, in
'ConvParam' (callproc.c), this integer ends up as a 4 byte integer type
on the argument stack for ffi. I don't know anything about ffi, but this
looks at least suspicious on any platform where sizeof(void*) is 8.

I propose to remove NULL pointer handling from the above from_param
function, since Py_None is properly handled by ConvParam itself. A patch
against the 2.6 maintenance branch is attached.

[robertluce]

Thomas, is there any chance of getting your attention for this one?
Deciding whether or not this issue can be fully resolved by applying the
proposed patch would already be sufficient. If it is not, I am willing
to invest more time on resolving this issue.

[amcnabb]

I ran into this problem, too. It took me a long time to track down the
segfaults. It's really bad to pass in None and have the system pick
some random address instead of 0.

I looked at the attached patch, and it seems to me the only alternative
approach would be to use PyLong_FromLong instead of PyInt_FromLong.
However, since ConvParam already handles None appropriately, I think the
fix in patch_ctypes_none_arg.diff really is the best way to do it.

This patch is a one-line fix (plus tests and documentation), and it
fixes a bug which crashes the interpreter. The patch seems very
straightforward, and there is no way that code could depend on the
current behavior. I'm not sure if my patch review counts for much, but
there you have it. :)

It would be great if this patch could be applied quickly and added to
the maintenance branch for 2.6. Thanks.

[theller]

Thanks for bringing my attention to this problem again, and for the review.

Andrew McNabb schrieb:

I looked at the attached patch, and it seems to me the only alternative
approach would be to use PyLong_FromLong instead of PyInt_FromLong.

Using PyLong_FromLong doesn't make a difference at all, if you look into
the ConvParam code.

However, since ConvParam already handles None appropriately, I think the
fix in patch_ctypes_none_arg.diff really is the best way to do it.

The problem is that the patch changes the behaviour of the
'POINTER(...).from_param' methods, so I'm unsure if it can be applied
to 2.6 (or even 2.5). Opinions?

This patch is a one-line fix (plus tests and documentation), and it
fixes a bug which crashes the interpreter. The patch seems very

The patch is missing a 'Py_INCREF(value)' before the 'return value;',
but this is a minor point.

straightforward, and there is no way that code could depend on the
current behavior.

It could (on 32-bit systems), although I'm not sure if there is code
that does.

I'm not sure if my patch review counts for much, but
there you have it. :)

[theller]

Here's a corrected patch against svn trunk.

[amcnabb]

On Thu, Jul 30, 2009 at 07:14:56PM +0000, Thomas Heller wrote:

Thanks for bringing my attention to this problem again, and for the review.

I'm just glad to help.

The problem is that the patch changes the behaviour of the
'POINTER(...).from_param' methods, so I'm unsure if it can be applied
to 2.6 (or even 2.5). Opinions?

Hmm. So you're saying that someone could be calling from_param() and
expecting to see 0 instead of None. Am I understanding correctly? I
suppose that could happen on either 32-bit or 64-bit systems (because
from_param returns 0 for both). I can't personally think of a situation
where someone would rely on that, but maybe it's something to ask about
on python-dev.

I just looked at ConvParam in a little more detail, and it seems that
the problem is caused because setting pa->value.i to 0 only works for
the lower-order bits. Apparently the higher order bits in pa->value are
non-zero when ConvParam is called. Would it be possible to zero-out
pa->value at the top of ConvParam (i.e., putting "bzero(&(pa->value),
sizeof(pa->value)" or something at the top of ConvParam)? Am I missing
something about what's in pa before ConvParam is called?

The patch is missing a 'Py_INCREF(value)' before the 'return value;',
but this is a minor point.

Thanks for pointing that out. I'm still getting familiar with reference
counting in the Python API.

[theller]

Andrew McNabb schrieb:

I just looked at ConvParam in a little more detail, and it seems that
the problem is caused because setting pa->value.i to 0 only works for
the lower-order bits. Apparently the higher order bits in pa->value are
non-zero when ConvParam is called. Would it be possible to zero-out
pa->value at the top of ConvParam (i.e., putting "bzero(&(pa->value),
sizeof(pa->value)" or something at the top of ConvParam)? Am I missing
something about what's in pa before ConvParam is called?

Zeroing out higher order bits wouldn't help because the parameter would
still be passed as int instead as pointer; see the ffi_type field.

However, after thinking the whole issue over for some time I'm now
convinced that this patch is the right approach; even if .from_param()
returns 'None' instead of '0' the former should be accepted as well
because it better represents a NULL pointer anyway.

So I will apply the patch to trunk and 2.6, and forward port to the
python3 branches. After further testing.

[theller]

Python 3.0 is dead ;-).

[theller]

Comitted as

trunk: 74921, py3k: 74922, release31-maint: 74923, release26-maint: 74924

[olt]

For anyone that has to use a Python version where this bug is present, it is possible to manually create a NULL pointer.

For the example:

POINTER(c_double)()

This works, at least in my case.