Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
robertluce mannequin opened this issue Dec 9, 2008 · 10 comments
Closed

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

robertluce mannequin opened this issue Dec 9, 2008 · 10 comments
Assignees
Labels
topic-ctypes type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@robertluce
Copy link
Mannequin

robertluce mannequin commented Dec 9, 2008

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
    Copy link
    Mannequin Author

    robertluce mannequin commented Dec 9, 2008

    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 robertluce mannequin added the type-bug An unexpected behavior, bug, or error label Dec 9, 2008
    @robertluce robertluce mannequin assigned theller Dec 9, 2008
    @robertluce robertluce mannequin added the topic-ctypes label Dec 9, 2008
    @robertluce
    Copy link
    Mannequin Author

    robertluce mannequin commented Feb 17, 2009

    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
    Copy link
    Mannequin

    amcnabb mannequin commented Jul 30, 2009

    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.

    @amcnabb amcnabb mannequin added type-crash A hard crash of the interpreter, possibly with a core dump and removed type-bug An unexpected behavior, bug, or error labels Jul 30, 2009
    @theller
    Copy link

    theller commented Jul 30, 2009

    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
    Copy link

    theller commented Jul 30, 2009

    Here's a corrected patch against svn trunk.

    @amcnabb
    Copy link
    Mannequin

    amcnabb mannequin commented Jul 30, 2009

    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
    Copy link

    theller commented Jul 31, 2009

    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
    Copy link

    theller commented Jul 31, 2009

    Python 3.0 is dead ;-).

    @theller
    Copy link

    theller commented Sep 18, 2009

    Comitted as

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

    @theller theller closed this as completed Sep 18, 2009
    @olt
    Copy link
    Mannequin

    olt mannequin commented Apr 11, 2011

    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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-ctypes type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant