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

Updating _fields_ of a derived struct type yields a bad cif #62260

Closed
laurialanko mannequin opened this issue May 25, 2013 · 7 comments
Closed

Updating _fields_ of a derived struct type yields a bad cif #62260

laurialanko mannequin opened this issue May 25, 2013 · 7 comments
Labels
3.11 only security fixes 3.12 bugs and security fixes 3.13 new features, bugs and security fixes topic-ctypes type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@laurialanko
Copy link
Mannequin

laurialanko mannequin commented May 25, 2013

BPO 18060
Nosy @amauryfa, @abalkin, @meadori, @websurfer5
PRs
  • bpo-18060: Updating _fields_ of a derived struct type yields a bad cif #13374
  • Files
  • t1.py: Test case
  • t2.py: test case uses PyCStructUnionType_update_stgdict() but not PyCStgDict_clone()
  • 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 = None
    created_at = <Date 2013-05-25.19:07:26.282>
    labels = ['ctypes', '3.7', '3.8', 'type-crash']
    title = 'Updating _fields_ of a derived struct type yields a bad cif'
    updated_at = <Date 2019-05-17.04:20:58.133>
    user = 'https://bugs.python.org/laurialanko'

    bugs.python.org fields:

    activity = <Date 2019-05-17.04:20:58.133>
    actor = 'Jeffrey.Kintscher'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['ctypes']
    creation = <Date 2013-05-25.19:07:26.282>
    creator = 'lauri.alanko'
    dependencies = []
    files = ['30369', '48332']
    hgrepos = []
    issue_num = 18060
    keywords = ['patch']
    message_count = 6.0
    messages = ['189992', '228414', '342505', '342520', '342532', '342676']
    nosy_count = 5.0
    nosy_names = ['amaury.forgeotdarc', 'belopolsky', 'meador.inge', 'lauri.alanko', 'Jeffrey.Kintscher']
    pr_nums = ['13374']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue18060'
    versions = ['Python 3.4', 'Python 3.5', 'Python 3.7', 'Python 3.8']

    Linked PRs

    @laurialanko
    Copy link
    Mannequin Author

    laurialanko mannequin commented May 25, 2013

    In Modules/_ctypes/stgdict.c:567 there is a suspicious line:

    stgdict->length = len;      /* ADD ffi_ofs? */
    

    That is, the length field of the stgdict is set to the number of fields in the immediate Structure class, and the number of fields in the parent class (ffi_ofs) is questionably left out. This is wrong.

    The length field is used in PyCStgDict_clone to copy the ffi_type descriptors for struct elements to a derived struct type. If length is short, not all element types are copied, and the resulting array is not NULL-terminated.

    So the problem manifests when you inherit from a structure type, update the _fields_ of the inherited type, and then inherit again from the updated type. Even then everything might seem normal, since the elements array is actually not used very much.

    However, attached is a test case that segfaults at least with debug builds on ARM with the VFP ABI. The non-null-terminated element type array is traversed to find if the structure can be passed in floating point registers, eventually resulting in dereferencing 0xfbfbfbfb.

    The test program should print out pi. To avoid the hassle of a separate C component, the program abuses the standard atan2 function by pretending it takes a struct of two doubles instead of two separate double parameters. This does not make a difference to the ABI.

    Fixing the bug is trivial. Just change the line to:

    stgdict->length = ffi_ofs + len;

    @laurialanko laurialanko mannequin added topic-ctypes type-crash A hard crash of the interpreter, possibly with a core dump labels May 25, 2013
    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Oct 4, 2014

    A one line change is given in msg189992 so is this correct or isn't it?

    @websurfer5
    Copy link
    Mannequin

    websurfer5 mannequin commented May 14, 2019

    It still behaves as described in the 3.7 and master branches. The proposed fix works. I will submit a pull request.

    @websurfer5 websurfer5 mannequin added 3.7 (EOL) end of life 3.8 only security fixes labels May 14, 2019
    @websurfer5
    Copy link
    Mannequin

    websurfer5 mannequin commented May 14, 2019

    While the fix works as advertised, it breaks a similar existing test case:

    ======================================================================
    ERROR: test_positional_args (ctypes.test.test_structures.StructureTestCase)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/Users/jeff/sandbox/src/python3.7/cpython/Lib/ctypes/test/test_structures.py", line 390, in test_positional_args
        z = Z(1, 2, 3, 4, 5, 6)
    IndexError: list index out of range

    Below is the relevant test code:

            class W(Structure):
                _fields_ = [("a", c_int), ("b", c_int)]
            class X(W):
                _fields_ = [("c", c_int)]
            class Y(X):
                pass
            class Z(Y):
                _fields_ = [("d", c_int), ("e", c_int), ("f", c_int)]
    
            z = Z(1, 2, 3, 4, 5, 6)

    It similar but slightly different from the provided test case:

            libm = CDLL(find_library('m'))
    
            class Base(Structure):
                _fields_ = [('y', c_double),
                            ('x', c_double)]
    
            class Mid(Base):
                pass
    
            Mid._fields_ = []
    
            class Vector(Mid): pass
    
            libm.atan2.argtypes = [Vector]
            libm.atan2.restype = c_double
    
            arg = Vector(y=0.0, x=-1.0)

    I will do some more digging.

    @websurfer5
    Copy link
    Mannequin

    websurfer5 mannequin commented May 15, 2019

    The current behavior,

    stgdict->length = len;
    

    sets the number of elements in the class excluding its base classes. The new behavior of

    stgdict->length = ffi_ofs + len;
    

    sets the total number of elements in both the class and its base classes.

    This works as intended as long as the child classes do not add any more elements.

    @websurfer5
    Copy link
    Mannequin

    websurfer5 mannequin commented May 16, 2019

    The t1.py test case calls both PyCStructUnionType_update_stgdict() and PyCStgDict_clone(), both of which are broken. The test case in t2.py is simpler than t1.py in that it only calls PyCStructUnionType_update_stgdict().

    PyCStructUnionType_update_stgdict() gets called whenever _fields_ gets assigned a new list of element tuples. The function is supposed to copy any inherited element pointers in parent classes and append the new elements. The element pointers array in each child class is supposed to be cumulative (i.e. parent class element pointers plus child class element pointers).

    _fields_ is represented by a StgDictObject structure, and the relevant member variables are 'ffi_type_pointer.elements', 'len', and 'size'. 'ffi_type_pointer.elements' is an array of ffi_type pointers padded with a NULL pointer at the end. 'size' is the number of bytes in the array excluding the padding. 'len' is the number of elements in the current class (i.e. excluding the elements in parent classes).

    PyCStructUnionType_update_stgdict() allocates a new 'ffi_type_pointer.elements' array by adding 1 to the sum of 'len' and the 'len' member of the parent class, then multiplying by sizeof(ffi_type *). This works just fine when there is a single parent class, but breaks when there are multiple levels of inheritance. For example:

       class Base(Structure):
          _fields_ = [('y', c_double),
                      ('x', c_double)]
    
        class Mid(Base):
            _fields_ = []
    
        class Vector(Mid):
            _fields_ = []

    PyCStructUnionType_update_stgdict() gets called for each of the three _fileds_ assignments. Assuming a pointer size of 8 bytes, Base has these values:

        ffi_type_pointer.elements = array of 3 pointers (x, y, and NULL padding).
        len = 2
        size = 16 (i.e. 2 * sizeof(ffi_type *))

    Mid has these values:

        ffi_type_pointer.elements = array of 3 pointers (x, y, and NULL padding).
        len = 0
        size = 16 (i.e. 2 * sizeof(ffi_type *))

    Vector has these values:

        ffi_type_pointer.elements = array of 1 pointer (x)
        len = 0
        size = 16 (i.e. 2 * sizeof(ffi_type *))

    Vector's 'len' and 'size' are correct, but 'ffi_type_pointer.elements' contains one element instead of three. Vector should have:

        ffi_type_pointer.elements = array of 3 pointers (x, y, and NULL padding).
        len = 0
        size = 16 (i.e. 2 * sizeof(ffi_type *))

    'ffi_type_pointer.elements' got truncated because PyCStructUnionType_update_stgdict() uses the parent class's 'len' field to determine the size of the new array to allocate. As can be seen, Mid's 'len' is zero, so a new array with one element gets allocated and copied (0 element pointers plus a trailing NULL pointer for padding). Notice that Vector's 'size' is correct because the value is calculated as Mid's 'size' plus zero (for zero elements being added in the child class).

    Similarly, PyCStgDict_clone() has the same problem because it also uses the similar calculations based on 'len' to determine the new 'ffi_type_pointer.elements' array size that gets allocated and copied.

    The solution proposed by lauri.alanko effectively redefines the 'len' member variable to be the total number of elements defined in the inheritance chain for _fields_. While this does fix the allocation/copying issue, it breaks other code that expects the 'len' variables in the parent and child classes to be distinct values instead of cumulative. For example (from StructureTestCase.test_positional_args() in Lib/ctypes/test/test_structures.py),

        class W(Structure):
            _fields_ = [("a", c_int), ("b", c_int)]
        class X(W):
            _fields_ = [("c", c_int)]
        class Y(X):
            pass
        class Z(Y):
            _fields_ = [("d", c_int), ("e", c_int), ("f", c_int)]
    
        z = Z(1, 2, 3, 4, 5, 6)

    will throw an exception because Z's 'len' will be 6 instead of the expected 3.

    A better solution is to use 'size' to allocate and copy 'ffi_type_pointer.elements' since its value is already properly calculated and propagated through inheritance.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @serhiy-storchaka
    Copy link
    Member

    The length field was used in two incompatible meanings. In PyCStgDict_clone() it was the total number of items in ffi_type_pointer.elements (excluding the trailing null). In PyCStructUnionType_update_stgdict() and _init_pos_args() it was the the number of items added in this class, not including inherited items. There are two ways to resolve this:

    1. Calculate the total number in PyCStgDict_clone() by traversing recursively base classes.
    2. Keep the total number in length and change PyCStructUnionType_update_stgdict() and _init_pos_args().

    I choose the second option as simpler in long term.

    serhiy-storchaka added a commit that referenced this issue Jan 1, 2024
    …3374)
    
    The length field of StgDictObject for Structure class contains now
    the total number of items in ffi_type_pointer.elements (excluding
    the trailing null).
    
    The old behavior of using the number of elements in the parent class can
    cause the array to be truncated when it is copied, especially when there
    are multiple layers of subclassing.
    
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jan 1, 2024
    … layers (pythonGH-13374)
    
    The length field of StgDictObject for Structure class contains now
    the total number of items in ffi_type_pointer.elements (excluding
    the trailing null).
    
    The old behavior of using the number of elements in the parent class can
    cause the array to be truncated when it is copied, especially when there
    are multiple layers of subclassing.
    
    (cherry picked from commit 5f3cc90)
    
    Co-authored-by: Jeffrey Kintscher <49998481+websurfer5@users.noreply.github.com>
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jan 1, 2024
    … layers (pythonGH-13374)
    
    The length field of StgDictObject for Structure class contains now
    the total number of items in ffi_type_pointer.elements (excluding
    the trailing null).
    
    The old behavior of using the number of elements in the parent class can
    cause the array to be truncated when it is copied, especially when there
    are multiple layers of subclassing.
    
    (cherry picked from commit 5f3cc90)
    
    Co-authored-by: Jeffrey Kintscher <49998481+websurfer5@users.noreply.github.com>
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    @serhiy-storchaka serhiy-storchaka added 3.11 only security fixes 3.12 bugs and security fixes 3.13 new features, bugs and security fixes and removed 3.8 only security fixes 3.7 (EOL) end of life labels Jan 1, 2024
    serhiy-storchaka added a commit that referenced this issue Jan 1, 2024
    GH-13374) (GH-113624)
    
    The length field of StgDictObject for Structure class contains now
    the total number of items in ffi_type_pointer.elements (excluding
    the trailing null).
    
    The old behavior of using the number of elements in the parent class can
    cause the array to be truncated when it is copied, especially when there
    are multiple layers of subclassing.
    
    (cherry picked from commit 5f3cc90)
    
    Co-authored-by: Jeffrey Kintscher <49998481+websurfer5@users.noreply.github.com>
    serhiy-storchaka added a commit that referenced this issue Jan 5, 2024
    GH-13374) (GH-113623)
    
    The length field of StgDictObject for Structure class contains now
    the total number of items in ffi_type_pointer.elements (excluding
    the trailing null).
    
    The old behavior of using the number of elements in the parent class can
    cause the array to be truncated when it is copied, especially when there
    are multiple layers of subclassing.
    
    (cherry picked from commit 5f3cc90)
    
    Co-authored-by: Jeffrey Kintscher <49998481+websurfer5@users.noreply.github.com>
    kulikjak pushed a commit to kulikjak/cpython that referenced this issue Jan 22, 2024
    …pythonGH-13374)
    
    The length field of StgDictObject for Structure class contains now
    the total number of items in ffi_type_pointer.elements (excluding
    the trailing null).
    
    The old behavior of using the number of elements in the parent class can
    cause the array to be truncated when it is copied, especially when there
    are multiple layers of subclassing.
    
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
    …pythonGH-13374)
    
    The length field of StgDictObject for Structure class contains now
    the total number of items in ffi_type_pointer.elements (excluding
    the trailing null).
    
    The old behavior of using the number of elements in the parent class can
    cause the array to be truncated when it is copied, especially when there
    are multiple layers of subclassing.
    
    Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes 3.12 bugs and security fixes 3.13 new features, bugs and security fixes 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