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

Possible cause of segfault in subclassing #680

Closed
kngwyu opened this issue Dec 1, 2019 · 2 comments
Closed

Possible cause of segfault in subclassing #680

kngwyu opened this issue Dec 1, 2019 · 2 comments
Assignees
Labels

Comments

@kngwyu
Copy link
Member

kngwyu commented Dec 1, 2019

Related to #220, #407
cc: @konstin @joar @davidhewitt

So as I said now I'm investigating safer layout than our current pyclass, which is constructed (roughly) by

let ptr = alloc(...);  // *mut PyObject
let align = std::mem::align_of::<Self>();  // Self = pyclass
// Suppose 
// struct PyClassLayout { |\
//       base: Self::BASE,      | offset
//       self_: PyClass   |/
// }
let offset = (self::BASE::SIZE + align - 1) / align * align;  
let self_ptr = (ptr as *mut u8).offset(Self::OFFSET) as *mut T;

And what I found problematic is <Py~ as PyTypeInfo>::SIZE is std::mem::size_of::<ffi::PyObject> for all Py~ types, which is not true.
Actually, PyDictObject is defined as (roughly)

typedef struct {
    PyObject ob_base;
    Py_ssize_t ma_used;
    uint64_t ma_version_tag;
    PyDictKeysObject *ma_keys;
    PyObject **ma_values;
} PyDictObject;

So it's size 64bit * 4 = 32bytes larger than PyObject.
Is it the cause of segfault?

@kngwyu kngwyu added the bug label Dec 1, 2019
@kngwyu kngwyu self-assigned this Dec 2, 2019
@davidhewitt
Copy link
Member

It's certainly suspicious that the SIZE is incorrectly defined. However, I can't see where SIZE is actually used for PyDict. The only use I find of ::SIZE is in type_object::initialize_type(), which I don't think ever gets called for native types.

type_object.tp_basicsize = <T as PyTypeInfo>::SIZE as ffi::Py_ssize_t;

I would think that the macro in types/mod.rs should perhaps be calculating size differently:

const SIZE: usize = ::std::mem::size_of::<$crate::ffi::PyObject>();

I think SIZE really wants to be (*$typeobject).tp_basicsize? But then it can no longer be a constant.

I see in your upcoming branch #683 you've removed SIZE completely, which is also fine. I'll get round to reviewing that PR asap.

@davidhewitt
Copy link
Member

@kngwyu I guess this can now be closed thanks to your new PyClassShell!

@kngwyu kngwyu closed this as completed Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants