Give better errors on size changes of PyVarObjects and reduce false positives (GH-4869)

Fixes https://github.com/cython/cython/issues/4827

Some of the patch was copied from https://src.fedoraproject.org/rpms/Cython/pull-request/35#request_diff

Allows the size of a type to be between basicsize and basicsize+itemsize
since anything is this range is a reasonable size for a class to be, subject to
implementations details of the object struct.

Adds an explicit runtime test when an extern extension type is inherited from
to make sure that it isn't a PyVarObject of unexpected size.

10 files changed, 214 insertions, 47 deletions

diff --git a/Cython/Compiler/Nodes.py b/Cython/Compiler/Nodes.py
index 298cc5705..751eb31f4 100644
--- a/Cython/Compiler/Nodes.py
+++ b/Cython/Compiler/Nodes.py
@@ -5242,7 +5242,8 @@ class CClassDefNode(ClassDefNode):
                 error(base.pos, "Base class '%s' of type '%s' is final" % (
                     base_type, self.class_name))
             elif base_type.is_builtin_type and \
-                     base_type.name in ('tuple', 'str', 'bytes'):
+                     base_type.name in ('tuple', 'bytes'):
+                     # str in Py2 is also included in this, but now checked at run-time
                 error(base.pos, "inheritance from PyVarObject types like '%s' is not currently supported"
                       % base_type.name)
             else:
@@ -5511,6 +5512,22 @@ class CClassDefNode(ClassDefNode):
             ))
             code.putln("#endif")  # if CYTHON_USE_TYPE_SPECS
 
+            base_type = type.base_type
+            while base_type:
+                if base_type.is_external and not base_type.objstruct_cname == "PyTypeObject":
+                    # 'type' is special-cased because it is actually based on PyHeapTypeObject
+                    # Variable length bases are allowed if the current class doesn't grow
+                    code.putln("if (sizeof(%s%s) != sizeof(%s%s)) {" % (
+                        "" if type.typedef_flag else "struct ", type.objstruct_cname,
+                        "" if base_type.typedef_flag else "struct ", base_type.objstruct_cname))
+                    code.globalstate.use_utility_code(
+                        UtilityCode.load_cached("ValidateExternBase", "ExtensionTypes.c"))
+                    code.put_error_if_neg(entry.pos, "__Pyx_validate_extern_base(%s)" % (
+                        type.base_type.typeptr_cname))
+                    code.putln("}")
+                    break
+                base_type = base_type.base_type
+
             code.putln("#if !CYTHON_COMPILING_IN_LIMITED_API")
             # FIXME: these still need to get initialised even with the limited-API
             for slot in TypeSlots.get_slot_table(code.globalstate.directives):
diff --git a/Cython/Compiler/PyrexTypes.py b/Cython/Compiler/PyrexTypes.py
index 1316edddc..79e144ed1 100644
--- a/Cython/Compiler/PyrexTypes.py
+++ b/Cython/Compiler/PyrexTypes.py
@@ -1506,7 +1506,6 @@ class PyExtensionType(PyObjectType):
     #
     #  name             string
     #  scope            CClassScope      Attribute namespace
-    #  visibility       string
     #  typedef_flag     boolean
     #  base_type        PyExtensionType or None
     #  module_name      string or None   Qualified name of defining module
@@ -1520,6 +1519,7 @@ class PyExtensionType(PyObjectType):
     #  vtable_cname     string           Name of C method table definition
     #  early_init       boolean          Whether to initialize early (as opposed to during module execution).
     #  defered_declarations [thunk]      Used to declare class hierarchies in order
+    #  is_external      boolean          Defined in a extern block
     #  check_size       'warn', 'error', 'ignore'    What to do if tp_basicsize does not match
     #  dataclass_fields  OrderedDict nor None   Used for inheriting from dataclasses
 
diff --git a/Cython/Utility/ExtensionTypes.c b/Cython/Utility/ExtensionTypes.c
index ec994a367..aa39a860a 100644
--- a/Cython/Utility/ExtensionTypes.c
+++ b/Cython/Utility/ExtensionTypes.c
@@ -564,3 +564,37 @@ static PyObject *{{func_name}}(PyObject *left, PyObject *right {{extra_arg_decl}
     }
     return __Pyx_NewRef(Py_NotImplemented);
 }
+
+/////////////// ValidateExternBase.proto ///////////////
+
+static int __Pyx_validate_extern_base(PyTypeObject *base); /* proto */
+
+/////////////// ValidateExternBase ///////////////
+//@requires: ObjectHandling.c::FormatTypeName
+
+static int __Pyx_validate_extern_base(PyTypeObject *base) {
+    Py_ssize_t itemsize;
+#if CYTHON_COMPILING_IN_LIMITED_API
+    PyObject *py_itemsize;
+#endif
+#if !CYTHON_COMPILING_IN_LIMITED_API
+    itemsize = ((PyTypeObject *)base)->tp_itemsize;
+#else
+    py_itemsize = PyObject_GetAttrString(base, "__itemsize__");
+    if (!py_itemsize)
+        return -1;
+    itemsize = PyLong_AsSsize_t(py_itemsize);
+    Py_DECREF(py_itemsize);
+    py_itemsize = 0;
+    if (itemsize == (Py_ssize_t)-1 && PyErr_Occurred())
+        return -1;
+#endif
+    if (itemsize) {
+        __Pyx_TypeName b_name = __Pyx_PyType_GetName(base);
+        PyErr_Format(PyExc_TypeError,
+                "inheritance from PyVarObject types like '" __Pyx_FMT_TYPENAME "' not currently supported", b_name);
+        __Pyx_DECREF_TypeName(b_name);
+        return -1;
+    }
+    return 0;
+}
diff --git a/Cython/Utility/ImportExport.c b/Cython/Utility/ImportExport.c
index 6ceba7efb..897657281 100644
--- a/Cython/Utility/ImportExport.c
+++ b/Cython/Utility/ImportExport.c
@@ -498,8 +498,10 @@ static PyTypeObject *__Pyx_ImportType(PyObject *module, const char *module_name,
     PyObject *result = 0;
     char warning[200];
     Py_ssize_t basicsize;
+    Py_ssize_t itemsize;
 #if CYTHON_COMPILING_IN_LIMITED_API
     PyObject *py_basicsize;
+    PyObject *py_itemsize;
 #endif
 
     result = PyObject_GetAttrString(module, class_name);
@@ -513,6 +515,7 @@ static PyTypeObject *__Pyx_ImportType(PyObject *module, const char *module_name,
     }
 #if !CYTHON_COMPILING_IN_LIMITED_API
     basicsize = ((PyTypeObject *)result)->tp_basicsize;
+    itemsize = ((PyTypeObject *)result)->tp_itemsize;
 #else
     py_basicsize = PyObject_GetAttrString(result, "__basicsize__");
     if (!py_basicsize)
@@ -522,19 +525,30 @@ static PyTypeObject *__Pyx_ImportType(PyObject *module, const char *module_name,
     py_basicsize = 0;
     if (basicsize == (Py_ssize_t)-1 && PyErr_Occurred())
         goto bad;
+    py_itemsize = PyObject_GetAttrString(result, "__itemsize__");
+    if (!py_itemsize)
+        goto bad;
+    itemsize = PyLong_AsSsize_t(py_itemsize);
+    Py_DECREF(py_itemsize);
+    py_itemsize = 0;
+    if (itemsize == (Py_ssize_t)-1 && PyErr_Occurred())
+        goto bad;
 #endif
-    if ((size_t)basicsize < size) {
+    if ((size_t)(basicsize + itemsize) < size) {
         PyErr_Format(PyExc_ValueError,
             "%.200s.%.200s size changed, may indicate binary incompatibility. "
             "Expected %zd from C header, got %zd from PyObject",
-            module_name, class_name, size, basicsize);
+            module_name, class_name, size, basicsize+itemsize);
         goto bad;
     }
-    if (check_size == __Pyx_ImportType_CheckSize_Error && (size_t)basicsize != size) {
+    // varobjects almost have structs  between basicsize and basicsize + itemsize
+    // but the struct isn't always one of the two limiting values
+    if (check_size == __Pyx_ImportType_CheckSize_Error &&
+            ((size_t)basicsize > size || (size_t)(basicsize + itemsize) < size)) {
         PyErr_Format(PyExc_ValueError,
             "%.200s.%.200s size changed, may indicate binary incompatibility. "
-            "Expected %zd from C header, got %zd from PyObject",
-            module_name, class_name, size, basicsize);
+            "Expected %zd from C header, got %zd-%zd from PyObject",
+            module_name, class_name, size, basicsize, basicsize+itemsize);
         goto bad;
     }
     else if (check_size == __Pyx_ImportType_CheckSize_Warn && (size_t)basicsize > size) {
diff --git a/runtests.py b/runtests.py
index 72608882a..a5c12e65e 100755
--- a/runtests.py
+++ b/runtests.py
@@ -467,6 +467,7 @@ VER_DEP_MODULES = {
                                         'compile.extsetslice',
                                         'compile.extdelslice',
                                         'run.special_methods_T561_py2',
+                                        'run.builtin_type_inheritance_T608_py2only',
                                         ]),
     (3,3) : (operator.lt, lambda x: x in ['build.package_compilation',
                                           'build.cythonize_pep420_namespace',
diff --git a/tests/errors/builtin_type_inheritance.pyx b/tests/errors/builtin_type_inheritance.pyx
index 1c6ad31e1..a85f7a133 100644
--- a/tests/errors/builtin_type_inheritance.pyx
+++ b/tests/errors/builtin_type_inheritance.pyx
@@ -8,11 +8,9 @@ cdef class MyTuple(tuple):
 cdef class MyBytes(bytes):
     pass
 
-cdef class MyStr(str): # only in Py2, but can't know that during compilation
-    pass
+# str is also included in this in Py2, but checked at runtime instead
 
 _ERRORS = """
 5:19: inheritance from PyVarObject types like 'tuple' is not currently supported
 8:19: inheritance from PyVarObject types like 'bytes' is not currently supported
-11:17: inheritance from PyVarObject types like 'str' is not currently supported
 """
diff --git a/tests/pypy_bugs.txt b/tests/pypy_bugs.txt
index 1004a93e4..5a27265ee 100644
--- a/tests/pypy_bugs.txt
+++ b/tests/pypy_bugs.txt
@@ -61,3 +61,6 @@ run.exttype_dealloc
 # bugs in cpyext
 run.special_methods_T561
 run.special_methods_T561_py2
+
+# unicode is a PyVarObject on PyPy3
+run.builtin_type_inheritance_T608
diff --git a/tests/run/builtin_type_inheritance_T608.pyx b/tests/run/builtin_type_inheritance_T608.pyx
index 1214b6841..d03558a25 100644
--- a/tests/run/builtin_type_inheritance_T608.pyx
+++ b/tests/run/builtin_type_inheritance_T608.pyx
@@ -1,42 +1,6 @@
 # ticket: t608
 
-cdef class MyInt(int):
-    """
-    >>> MyInt(2) == 2
-    True
-    >>> MyInt(2).attr is None
-    True
-    """
-    cdef readonly object attr
-
-cdef class MyInt2(int):
-    """
-    >>> MyInt2(2) == 2
-    True
-    >>> MyInt2(2).attr is None
-    True
-    >>> MyInt2(2).test(3)
-    5
-    """
-    cdef readonly object attr
-
-    def test(self, arg):
-        return self._test(arg)
-
-    cdef _test(self, arg):
-        return self + arg
-
-cdef class MyInt3(MyInt2):
-    """
-    >>> MyInt3(2) == 2
-    True
-    >>> MyInt3(2).attr is None
-    True
-    >>> MyInt3(2).test(3)
-    6
-    """
-    cdef _test(self, arg):
-        return self + arg + 1
+# see "builtin_type_inheritance_T608_py2only.pyx" for inheritance from int
 
 cdef class MyFloat(float):
     """
diff --git a/tests/run/builtin_type_inheritance_T608_py2only.pyx b/tests/run/builtin_type_inheritance_T608_py2only.pyx
new file mode 100644
index 000000000..b10a2610a
--- /dev/null
+++ b/tests/run/builtin_type_inheritance_T608_py2only.pyx
@@ -0,0 +1,42 @@
+# ticket: t608
+
+# This only works reliably in Python2. In Python3 ints are variable-sized.
+# You get away with it for small ints but it's a bad idea
+
+cdef class MyInt(int):
+    """
+    >>> MyInt(2) == 2
+    True
+    >>> MyInt(2).attr is None
+    True
+    """
+    cdef readonly object attr
+
+cdef class MyInt2(int):
+    """
+    >>> MyInt2(2) == 2
+    True
+    >>> MyInt2(2).attr is None
+    True
+    >>> MyInt2(2).test(3)
+    5
+    """
+    cdef readonly object attr
+
+    def test(self, arg):
+        return self._test(arg)
+
+    cdef _test(self, arg):
+        return self + arg
+
+cdef class MyInt3(MyInt2):
+    """
+    >>> MyInt3(2) == 2
+    True
+    >>> MyInt3(2).attr is None
+    True
+    >>> MyInt3(2).test(3)
+    6
+    """
+    cdef _test(self, arg):
+        return self + arg + 1
diff --git a/tests/run/extern_varobject_extensions.srctree b/tests/run/extern_varobject_extensions.srctree
new file mode 100644
index 000000000..c927b8147
--- /dev/null
+++ b/tests/run/extern_varobject_extensions.srctree
@@ -0,0 +1,94 @@
+# mode: run
+
+PYTHON setup.py build_ext --inplace
+PYTHON -c "import classes"
+PYTHON -c "import test_inherit"
+
+######## setup.py ########
+
+from Cython.Build.Dependencies import cythonize
+
+from distutils.core import setup
+
+setup(
+  ext_modules=cythonize("*.pyx"),
+)
+
+######  dummy_module.py ###########
+
+tpl = tuple
+lst = list
+
+###### classes.pxd ################
+
+cdef extern from *:
+    # apart from list, these are all variable sized types
+    # and Cython shouldn't trip up about the struct size
+    ctypedef class dummy_module.tpl [object PyTupleObject]:
+        pass
+    ctypedef class dummy_module.lst [object PyListObject]:
+        pass
+    ctypedef class types.CodeType [object PyCodeObject]:
+        pass
+    # Note that bytes doesn't work here because it further 
+    # the tp_basicsize to save space
+        
+##### classes.pyx #################
+
+def check_tuple(tpl x):
+    assert isinstance(x, tuple)
+    
+def check_list(lst x):
+    assert isinstance(x, list)
+    
+def check_code(CodeType x):
+    import types
+    assert isinstance(x, types.CodeType)
+    
+check_tuple((1, 2))
+check_list([1, 2])
+check_code(eval("lambda: None").__code__)
+
+##### failed_inherit1.pyx #############
+
+from classes cimport tpl
+
+cdef class SuperTuple(tpl):
+    cdef int a  # importing this gives an error message
+
+##### failed_inherit2.pyx #############
+
+from classes cimport tpl
+
+cdef class SuperTuple(tpl):
+    # adding a method creates a vtab so should also fail
+    cdef int func(self):
+        return 1
+    
+##### successful_inherit.pyx #########
+
+from classes cimport lst, tpl
+
+cdef class SuperList(lst):
+    cdef int a  # This works OK
+    
+cdef class SuperTuple(tpl):
+    # This is actually OK because it doesn't add anything
+    pass
+    
+##### test_inherit.py ################
+
+try:
+    import failed_inherit1
+except TypeError as e:
+    assert e.args[0] == "inheritance from PyVarObject types like 'tuple' not currently supported", e.args[0]
+else:
+    assert False
+try:
+    import failed_inherit2
+except TypeError as e:
+    assert e.args[0] == "inheritance from PyVarObject types like 'tuple' not currently supported", e.args[0]
+else:
+    assert False
+    
+import successful_inherit