classification
Title: Passing 'None' if argtype is set to POINTER(...) doesn't always result in NULL
Type: crash Stage:
Components: ctypes Versions: Python 3.2, Python 3.1, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: theller Nosy List: LambertDW, amcnabb, olt, robertluce, theller
Priority: normal Keywords: patch

Created on 2008-12-09 09:46 by robertluce, last changed 2011-04-11 12:49 by olt. This issue is now closed.

Files
File name Uploaded Description Edit
patch_ctypes_none_arg.diff robertluce, 2008-12-09 09:46 proposed patch against 2.6
patch-ctypes-none-arg-2.diff theller, 2009-07-30 19:17 New patch against trunk, added the missing Py_INCREF().
Messages (10)
msg77397 - (view) Author: Robert Luce (robertluce) Date: 2008-12-09 09:46
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.
msg82371 - (view) Author: Robert Luce (robertluce) Date: 2009-02-17 21:37
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.
msg91082 - (view) Author: Andrew McNabb (amcnabb) Date: 2009-07-30 03:59
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.
msg91103 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2009-07-30 19:14
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. :)
msg91104 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2009-07-30 19:17
Here's a corrected patch against svn trunk.
msg91112 - (view) Author: Andrew McNabb (amcnabb) Date: 2009-07-30 20:43
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.
msg91144 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2009-07-31 19:34
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.
msg91145 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2009-07-31 19:35
Python 3.0 is dead ;-).
msg92841 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2009-09-18 20:13
Comitted as

trunk: 74921, py3k: 74922, release31-maint: 74923, release26-maint: 74924
msg133518 - (view) Author: (olt) Date: 2011-04-11 12:49
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.
History
Date User Action Args
2011-04-11 12:49:47oltsetnosy: + olt
messages: + msg133518
2009-09-18 20:13:47thellersetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg92841
2009-07-31 19:35:24thellersetmessages: + msg91145
versions: - Python 3.0
2009-07-31 19:34:31thellersetresolution: accepted
messages: + msg91144
versions: + Python 3.0, Python 3.1, Python 2.7, Python 3.2, - Python 2.5
2009-07-30 20:43:03amcnabbsetmessages: + msg91112
2009-07-30 19:17:37thellersetfiles: + patch-ctypes-none-arg-2.diff

messages: + msg91104
2009-07-30 19:14:55thellersetmessages: + msg91103
2009-07-30 03:59:45amcnabbsetnosy: + amcnabb
type: behavior -> crash
messages: + msg91082
2009-02-17 21:37:36robertlucesetmessages: + msg82371
2008-12-09 14:56:59LambertDWsetnosy: + LambertDW
2008-12-09 09:46:59robertlucecreate