| 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) *  |
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) * |
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) * |
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) * |
Date: 2009-07-31 19:35 |
| Python 3.0 is dead ;-). |
|
|
| msg92841 - (view) |
Author: Thomas Heller (theller) * |
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. |
|
|