From 06593fb0f23dfc12f482561fbec1717dca0d4db4 Mon Sep 17 00:00:00 2001
From: Damien George <damien.p.george@gmail.com>
Date: Fri, 19 Jun 2015 12:49:10 +0000
Subject: [PATCH] py: Use a wrapper to explicitly check self argument of
 builtin methods.

Previous to this patch a call such as list.append(1, 2) would lead to a
seg fault.  This is because list.append is a builtin method and the first
argument to such methods is always assumed to have the correct type.

Now, when a builtin method is extracted like this it is wrapped in a
checker object which checks the the type of the first argument before
calling the builtin function.

This feature is contrelled by MICROPY_BUILTIN_METHOD_CHECK_SELF_ARG and
is enabled by default.

See issue #1216.
---
 bare-arm/mpconfigport.h         |  1 +
 minimal/mpconfigport.h          |  1 +
 py/mpconfig.h                   |  9 +++++
 py/runtime.c                    | 60 +++++++++++++++++++++++++++++++--
 tests/basics/class_use_other.py | 12 +++++++
 tests/basics/self_type_check.py | 31 +++++++++++++++++
 unix/mpconfigport_minimal.h     |  1 +
 7 files changed, 112 insertions(+), 3 deletions(-)
 create mode 100644 tests/basics/class_use_other.py
 create mode 100644 tests/basics/self_type_check.py

diff --git a/bare-arm/mpconfigport.h b/bare-arm/mpconfigport.h
index 41fe5dace..cae27cfd9 100644
--- a/bare-arm/mpconfigport.h
+++ b/bare-arm/mpconfigport.h
@@ -18,6 +18,7 @@
 #define MICROPY_ENABLE_SOURCE_LINE  (0)
 #define MICROPY_ENABLE_DOC_STRING   (0)
 #define MICROPY_ERROR_REPORTING     (MICROPY_ERROR_REPORTING_TERSE)
+#define MICROPY_BUILTIN_METHOD_CHECK_SELF_ARG (0)
 #define MICROPY_PY_BUILTINS_BYTEARRAY (0)
 #define MICROPY_PY_BUILTINS_MEMORYVIEW (0)
 #define MICROPY_PY_BUILTINS_FROZENSET (0)
diff --git a/minimal/mpconfigport.h b/minimal/mpconfigport.h
index ef4fc00b4..49f380ea0 100644
--- a/minimal/mpconfigport.h
+++ b/minimal/mpconfigport.h
@@ -19,6 +19,7 @@
 #define MICROPY_ENABLE_SOURCE_LINE  (0)
 #define MICROPY_ENABLE_DOC_STRING   (0)
 #define MICROPY_ERROR_REPORTING     (MICROPY_ERROR_REPORTING_TERSE)
+#define MICROPY_BUILTIN_METHOD_CHECK_SELF_ARG (0)
 #define MICROPY_PY_BUILTINS_BYTEARRAY (0)
 #define MICROPY_PY_BUILTINS_MEMORYVIEW (0)
 #define MICROPY_PY_BUILTINS_ENUMERATE (0)
diff --git a/py/mpconfig.h b/py/mpconfig.h
index 2d9288f32..9f541ef70 100644
--- a/py/mpconfig.h
+++ b/py/mpconfig.h
@@ -393,6 +393,15 @@ typedef double mp_float_t;
 #define MICROPY_CAN_OVERRIDE_BUILTINS (0)
 #endif
 
+// Whether to check that the "self" argument of a builtin method has the
+// correct type.  Such an explicit check is only needed if a builtin
+// method escapes to Python land without a first argument, eg
+// list.append([], 1).  Without this check such calls will have undefined
+// behaviour (usually segfault) if the first argument is the wrong type.
+#ifndef MICROPY_BUILTIN_METHOD_CHECK_SELF_ARG
+#define MICROPY_BUILTIN_METHOD_CHECK_SELF_ARG (1)
+#endif
+
 /*****************************************************************************/
 /* Fine control over Python builtins, classes, modules, etc                  */
 
diff --git a/py/runtime.c b/py/runtime.c
index 501ca435a..4959617ef 100644
--- a/py/runtime.c
+++ b/py/runtime.c
@@ -887,6 +887,51 @@ mp_obj_t mp_load_attr(mp_obj_t base, qstr attr) {
     }
 }
 
+#if MICROPY_BUILTIN_METHOD_CHECK_SELF_ARG
+
+// The following "checked fun" type is local to the mp_convert_member_lookup
+// function, and serves to check that the first argument to a builtin function
+// has the correct type.
+
+typedef struct _mp_obj_checked_fun_t {
+    mp_obj_base_t base;
+    const mp_obj_type_t *type;
+    mp_obj_t fun;
+} mp_obj_checked_fun_t;
+
+STATIC mp_obj_t checked_fun_call(mp_obj_t self_in, mp_uint_t n_args, mp_uint_t n_kw, const mp_obj_t *args) {
+    mp_obj_checked_fun_t *self = self_in;
+    if (n_args > 0) {
+        const mp_obj_type_t *arg0_type = mp_obj_get_type(args[0]);
+        if (arg0_type != self->type) {
+            if (MICROPY_ERROR_REPORTING != MICROPY_ERROR_REPORTING_DETAILED) {
+                nlr_raise(mp_obj_new_exception_msg(&mp_type_TypeError,
+                    "argument has wrong type"));
+            } else {
+                nlr_raise(mp_obj_new_exception_msg_varg(&mp_type_TypeError,
+                    "argument should be a '%q' not a '%q'", self->type->name, arg0_type->name));
+            }
+        }
+    }
+    return mp_call_function_n_kw(self->fun, n_args, n_kw, args);
+}
+
+STATIC const mp_obj_type_t mp_type_checked_fun = {
+    { &mp_type_type },
+    .name = MP_QSTR_function,
+    .call = checked_fun_call,
+};
+
+STATIC mp_obj_t mp_obj_new_checked_fun(const mp_obj_type_t *type, mp_obj_t fun) {
+    mp_obj_checked_fun_t *o = m_new_obj(mp_obj_checked_fun_t);
+    o->base.type = &mp_type_checked_fun;
+    o->type = type;
+    o->fun = fun;
+    return o;
+}
+
+#endif // MICROPY_BUILTIN_METHOD_CHECK_SELF_ARG
+
 // Given a member that was extracted from an instance, convert it correctly
 // and put the result in the dest[] array for a possible method call.
 // Conversion means dealing with static/class methods, callables, and values.
@@ -903,9 +948,18 @@ void mp_convert_member_lookup(mp_obj_t self, const mp_obj_type_t *type, mp_obj_t
         // Don't try to bind types (even though they're callable)
         dest[0] = member;
     } else if (mp_obj_is_callable(member)) {
-        // return a bound method, with self being this object
-        dest[0] = member;
-        dest[1] = self;
+        #if MICROPY_BUILTIN_METHOD_CHECK_SELF_ARG
+        if (self == MP_OBJ_NULL && mp_obj_get_type(member) == &mp_type_fun_builtin) {
+            // we extracted a builtin method without a first argument, so we must
+            // wrap this function in a type checker
+            dest[0] = mp_obj_new_checked_fun(type, member);
+        } else
+        #endif
+        {
+            // return a bound method, with self being this object
+            dest[0] = member;
+            dest[1] = self;
+        }
     } else {
         // class member is a value, so just return that value
         dest[0] = member;
diff --git a/tests/basics/class_use_other.py b/tests/basics/class_use_other.py
new file mode 100644
index 000000000..c6aa94ad2
--- /dev/null
+++ b/tests/basics/class_use_other.py
@@ -0,0 +1,12 @@
+# check that we can use an instance of B in a method of A
+
+class A:
+    def store(a, b):
+        a.value = b
+
+class B:
+    pass
+
+b = B()
+A.store(b, 1)
+print(b.value)
diff --git a/tests/basics/self_type_check.py b/tests/basics/self_type_check.py
new file mode 100644
index 000000000..947e362cd
--- /dev/null
+++ b/tests/basics/self_type_check.py
@@ -0,0 +1,31 @@
+# make sure type of first arg (self) to a builtin method is checked
+
+list.append
+
+try:
+    list.append()
+except TypeError as e:
+    print("TypeError")
+
+try:
+    list.append(1)
+except TypeError as e:
+    print("TypeError")
+
+try:
+    list.append(1, 2)
+except TypeError as e:
+    print("TypeError")
+
+l = []
+list.append(l, 2)
+print(l)
+
+try:
+    getattr(list, "append")(1, 2)
+except TypeError as e:
+    print("TypeError")
+
+l = []
+getattr(list, "append")(l, 2)
+print(l)
diff --git a/unix/mpconfigport_minimal.h b/unix/mpconfigport_minimal.h
index 078f5a1d1..770188a60 100644
--- a/unix/mpconfigport_minimal.h
+++ b/unix/mpconfigport_minimal.h
@@ -44,6 +44,7 @@
 #define MICROPY_OPT_COMPUTED_GOTO   (0)
 #define MICROPY_OPT_CACHE_MAP_LOOKUP_IN_BYTECODE (0)
 #define MICROPY_CAN_OVERRIDE_BUILTINS (0)
+#define MICROPY_BUILTIN_METHOD_CHECK_SELF_ARG (0)
 #define MICROPY_CPYTHON_COMPAT      (0)
 #define MICROPY_PY_BUILTINS_BYTEARRAY (0)
 #define MICROPY_PY_BUILTINS_MEMORYVIEW (0)
-- 
GitLab