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

Resize doesn't change reported length on create_string_buffer() #65527

Closed
DustinOprea mannequin opened this issue Apr 22, 2014 · 8 comments
Closed

Resize doesn't change reported length on create_string_buffer() #65527

DustinOprea mannequin opened this issue Apr 22, 2014 · 8 comments
Labels
topic-ctypes type-feature A feature request or enhancement

Comments

@DustinOprea
Copy link
Mannequin

DustinOprea mannequin commented Apr 22, 2014

BPO 21328
Nosy @meadori, @vadmium, @eryksun
Files
  • resize.patch
  • 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 = None
    closed_at = <Date 2016-02-05.08:40:40.541>
    created_at = <Date 2014-04-22.14:02:29.612>
    labels = ['ctypes', 'type-feature', 'invalid']
    title = "Resize doesn't change reported length on create_string_buffer()"
    updated_at = <Date 2016-02-05.08:48:06.255>
    user = 'https://bugs.python.org/DustinOprea'

    bugs.python.org fields:

    activity = <Date 2016-02-05.08:48:06.255>
    actor = 'eryksun'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-02-05.08:40:40.541>
    closer = 'Dustin.Oprea'
    components = ['ctypes']
    creation = <Date 2014-04-22.14:02:29.612>
    creator = 'Dustin.Oprea'
    dependencies = []
    files = ['41810']
    hgrepos = []
    issue_num = 21328
    keywords = ['patch']
    message_count = 8.0
    messages = ['217005', '259575', '259600', '259604', '259606', '259624', '259638', '259639']
    nosy_count = 5.0
    nosy_names = ['meador.inge', 'martin.panter', 'eryksun', 'Dustin.Oprea', 'beng94']
    pr_nums = []
    priority = 'normal'
    resolution = 'not a bug'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue21328'
    versions = ['Python 2.7']

    @DustinOprea
    Copy link
    Mannequin Author

    DustinOprea mannequin commented Apr 22, 2014

    The memory is resized, but the value returned by len() doesn't change:

    >>> b = ctypes.create_string_buffer(23)
    >>> len(b)
    23
    >>> b.raw = '0' * 23
    >>> b.raw = '0' * 24
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ValueError: string too long
    
    >>> ctypes.resize(b, 28)
    >>> len(b)
    23
    >>> b.raw = '0' * 28
    >>> b.raw = '0' * 29
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ValueError: string too long

    @DustinOprea DustinOprea mannequin added the topic-ctypes label Apr 22, 2014
    @beng94
    Copy link
    Mannequin

    beng94 mannequin commented Feb 4, 2016

    I found out that if you modify Modules/_cpython/callproc.c resize function in a way that you set the obj->b_length to size, the len function returns the correct value.

    To be able to provide a proper patch, I'd like to look into len's implementation, can someone tell me where to look for it?

    @vadmium
    Copy link
    Member

    vadmium commented Feb 4, 2016

    I’m not sure if resize() should change the len(). Dustin, why do you think it should? Not all ctypes objects even implement len(). The len() of 10 seems embedded in the class of the return value:

    >>> b
    <ctypes.c_char_Array_10 object at 0x7f143362fd90>

    Also, how would this affect create_unicode_buffer(), if the buffer is resized to a non-multiple of sizeof(c_wchar)?

    Gedai: I’m not that familiar with the ctypes internals, but it looks like __len__() is implemented on the Array base class:

    >>> type(b).__len__
    <slot wrapper '__len__' of '_ctypes.Array' objects>

    Maybe look for the implementation of this method: <https://docs.python.org/3/c-api/typeobj.html#c.PySequenceMethods.sq_length\>.

    @beng94
    Copy link
    Mannequin

    beng94 mannequin commented Feb 4, 2016

    I've added a patch, that solves the problem with the built-in len. Even if it turns out that this functionality is not needed, it was quite of a challenge to track down the issue, I've learned a lot. :)

    Here are some functions, that I looked through, might be useful for someone, who'd like to look into this issue.

    https://github.com/python/cpython/blob/master/Python/bltinmodule.c#L1443
    static PyObject *
    builtin_len(PyModuleDef *module, PyObject *obj)
    /*[clinic end generated code: output=8e5837b6f81d915b input=bc55598da9e9c9b5]*/
    {
        Py_ssize_t res;
    
        res = PyObject_Size(obj);
        if (res < 0 && PyErr_Occurred())
            return NULL;
        return PyLong_FromSsize_t(res);
    }
    
    https://github.com/python/cpython/blob/master/Objects/abstract.c#L42
    Py_ssize_t
    PyObject_Size(PyObject *o)
    {
        /*...*/
        m = o->ob_type->tp_as_sequence;
        if (m && m->sq_length)
            return m->sq_length(o);
        /*...*/
    }

    https://github.com/python/cpython/blob/master/Modules/_ctypes/_ctypes.c#L4449
    static PySequenceMethods Array_as_sequence = {
    Array_length, /* sq_length; */
    /*...*/
    };

    https://github.com/python/cpython/blob/master/Modules/_ctypes/_ctypes.c#L4442
    static Py_ssize_t
    Array_length(PyObject *myself)
    {
        CDataObject *self = (CDataObject *)myself;
        return self->b_length;
    }

    @vadmium
    Copy link
    Member

    vadmium commented Feb 5, 2016

    I can’t really comment on the patch, but I’m a bit worried that this is not the purpose of the b_length field.

    @eryksun
    Copy link
    Contributor

    eryksun commented Feb 5, 2016

    You can't reassign the array object's __class__, and you can't modify the array type itself, so I think modifying the internal b_length field of the object is a confused result.

    Even if you ignore this confusion, it's still not as simple as using the size in bytes as the length. That's what b_size is for, after all. The array length is the new size divided by the element size, which you can get from PyType_stgdict(dict->proto)->size. Also, you'd have to ensure it's only updating b_length for arrays, i.e. ArrayObject_Check(obj), since it makes no sense to modify the length of a simple type, struct, union, or [function] pointer.

    However, I don't think this result is worth the confusion. ctypes buffers can grow, but arrays have a fixed size by design. There are already ways to access a resized buffer. For example, you can use the from_buffer method to create an instance of a new array type that has the desired length, or you can dereference a pointer for the new array type. So I'm inclined to close this issue as "not a bug".

    Note:
    Be careful when resizing buffers that are shared across objects. Say you resize array "a" and share it as array "b" using from_buffer or a pointer dereference. Then later you resize "a" again. The underlying realloc might change the base address of the buffer, while "b" still uses the old address. For example:

        >>> a = (ctypes.c_int * 5)(*range(5))
        >>> ctypes.resize(a, 4 * 10)
        >>> b = ctypes.POINTER(ctypes.c_int * 10)(a)[0]
        >>> ctypes.addressof(a)
        20136704
        >>> ctypes.addressof(b)
        20136704
        >>> b._b_needsfree_ # b doesn't own the buffer
        0
        >>> b[:] # the reallocation is not zeroed
        [0, 1, 2, 3, 4, 0, 0, 32736, 48, 0]

    Here's the problem to keep in mind:

        >>> ctypes.resize(a, 4 * 1000)
        >>> ctypes.addressof(a)
        22077952
        >>> ctypes.addressof(b)
        20136704
        >>> b[:] # garbage; maybe a segfault
        [1771815800, 32736, 1633841761, 540763495, 1652121965,
         543236197, 1718052211, 1953264993, 10, 0]

    @eryksun eryksun added the type-feature A feature request or enhancement label Feb 5, 2016
    @beng94
    Copy link
    Mannequin

    beng94 mannequin commented Feb 5, 2016

    Thanks for the remarks, I think the issue can be closed as well.

    @DustinOprea
    Copy link
    Mannequin Author

    DustinOprea mannequin commented Feb 5, 2016

    I'm closing it. The ticket has been open two-years and no one else seemed to be interested in this issue until now, which leads me to believe that it's a PEBCAK/understanding issue. The rationale for why it's irrelevant seems sound. Thanks for digging through the code, Tamás. Thank you for your comments, Martin and Eryk.

    @DustinOprea DustinOprea mannequin closed this as completed Feb 5, 2016
    @eryksun eryksun added the invalid label Feb 5, 2016
    @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-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants