Fix serious soundness error in JsProxy_Call kwargs handling

src/core/hiwire.c

@@ -376,6 +376,11 @@ EM_JS_NUM(bool, hiwire_is_function, (JsRef idobj), {
   // clang-format on
 });
 
+EM_JS_NUM(bool, hiwire_function_supports_kwargs, (JsRef idfunc), {
+  let funcstr = Module.hiwire.get_value(idfunc).toString();
+  return Module.function_supports_kwargs(funcstr);
+});
+
 EM_JS_NUM(bool, hiwire_is_promise, (JsRef idobj), {
   // clang-format off
   let obj = Module.hiwire.get_value(idobj);

src/core/hiwire.h

@@ -449,6 +449,9 @@ hiwire_get_bool(JsRef idobj);
 bool
 hiwire_is_function(JsRef idobj);
 
+bool
+hiwire_function_supports_kwargs(JsRef idfunc);
+
 /**
  * Returns true if the object is a promise.
  */

src/core/jsproxy.c

@@ -33,6 +33,8 @@ JsBoundMethod_cnew(JsRef this_, const char* name);
 typedef struct
 {
   PyObject_HEAD
+  vectorcallfunc vectorcall;
+  int supports_kwargs; // -1 : don't know. 0 : no, 1 : yes
   JsRef js;
   PyObject* bytes;
   bool awaited; // for promises
@@ -128,45 +130,67 @@ JsProxy_SetAttr(PyObject* self, PyObject* attr, PyObject* pyvalue)
   return success ? 0 : -1;
 }
 
+#define JsProxy_JSREF(x) (((JsProxy*)x)->js)
+#define JsProxy_SUPPORTS_KWARGS(x) (((JsProxy*)x)->supports_kwargs)
+
 static PyObject*
-JsProxy_Call(PyObject* self, PyObject* args, PyObject* kwargs)
-{
-  bool success = false;
-  JsRef idargs = NULL;
-  JsRef idarg = NULL;
-  JsRef idkwargs = NULL;
-  JsRef idresult = NULL;
-  // result:
-  PyObject* pyresult;
-  Py_ssize_t nargs = PyTuple_Size(args);
+JsProxy_Vectorcall(PyObject* self,
+                   PyObject* const* args,
+                   size_t nargsf,
+                   PyObject* kwnames)
+{
+  bool kwargs = false;
+  if (kwnames != NULL) {
+    // There were kwargs? But maybe kwnames is the empty tuple?
+    PyObject* kwname = PyTuple_GetItem(kwnames, 0);
+    // Clear IndexError
+    PyErr_Clear();
+    if (kwname != NULL) {
+      kwargs = true;
+      if (JsProxy_SUPPORTS_KWARGS(self) == -1) {
+        JsProxy_SUPPORTS_KWARGS(self) =
+          hiwire_function_supports_kwargs(JsProxy_JSREF(self));
+      }
+    }
+    if (kwargs && !JsProxy_SUPPORTS_KWARGS(self)) {
+      // We have kwargs but function doesn't support them. Raise error.
+      const char* kwname_utf8 = PyUnicode_AsUTF8(kwname);
+      PyErr_Format(PyExc_TypeError,
+                   "jsproxy got an unexpected keyword argument '%s'",
+                   kwname_utf8);
+      return NULL;
+    }
+  }
 
-  idargs = hiwire_array();
+  Py_EnterRecursiveCall(" in JsProxy_Vectorcall");
+
+  Py_ssize_t nargs = PyVectorcall_NARGS(nargsf);
+  JsRef idargs = hiwire_array();
   for (Py_ssize_t i = 0; i < nargs; ++i) {
-    idarg = python2js(PyTuple_GET_ITEM(args, i));
-    FAIL_IF_NULL(idarg);
-    FAIL_IF_MINUS_ONE(hiwire_push_array(idargs, idarg));
-    hiwire_CLEAR(idarg);
+    JsRef idarg = python2js(args[i]);
+    hiwire_push_array(idargs, idarg);
+    hiwire_decref(idarg);
   }
 
-  if (PyDict_Size(kwargs)) {
-    idkwargs = python2js(kwargs);
-    FAIL_IF_MINUS_ONE(hiwire_push_array(idargs, idkwargs));
+  if (kwargs) {
+    // store kwargs into an object which we'll use as the last argument.
+    JsRef idkwargs = hiwire_object();
+    Py_ssize_t nkwargs = PyTuple_Size(kwnames);
+    for (Py_ssize_t i = 0, k = nargsf; i < nkwargs; ++i, ++k) {
+      PyObject* name = PyTuple_GET_ITEM(kwnames, i);
+      const char* name_utf8 = PyUnicode_AsUTF8(name);
+      JsRef jsarg = python2js(args[k]);
+      hiwire_set_member_string(idkwargs, name_utf8, jsarg);
+    }
+    hiwire_push_array(idargs, idkwargs);
+    hiwire_decref(idkwargs);
   }
 
-  idresult = hiwire_call(JsProxy_REF(self), idargs);
-  FAIL_IF_NULL(idresult);
-  pyresult = js2python(idresult);
-  FAIL_IF_NULL(pyresult);
-
-  success = true;
-finally:
-  hiwire_CLEAR(idargs);
-  hiwire_CLEAR(idarg);
-  hiwire_CLEAR(idkwargs);
-  hiwire_CLEAR(idresult);
-  if (!success) {
-    Py_CLEAR(pyresult);
-  }
+  JsRef idresult = hiwire_call(JsProxy_JSREF(self), idargs);
+  hiwire_decref(idargs);
+  PyObject* pyresult = js2python(idresult);
+  hiwire_decref(idresult);
+  Py_LeaveRecursiveCall(/* " in JsProxy_Vectorcall" */);
   return pyresult;
 }
 
@@ -563,12 +587,13 @@ static PyTypeObject JsProxyType = {
   .tp_name = "JsProxy",
   .tp_basicsize = sizeof(JsProxy),
   .tp_dealloc = (destructor)JsProxy_dealloc,
-  .tp_call = JsProxy_Call,
+  .tp_call = PyVectorcall_Call,
+  .tp_vectorcall_offset = offsetof(JsProxy, vectorcall),
   .tp_getattro = JsProxy_GetAttr,
   .tp_setattro = JsProxy_SetAttr,
   .tp_as_async = &JsProxy_asyncMethods,
   .tp_richcompare = JsProxy_RichCompare,
-  .tp_flags = Py_TPFLAGS_DEFAULT,
+  .tp_flags = Py_TPFLAGS_DEFAULT | _Py_TPFLAGS_HAVE_VECTORCALL,
   .tp_doc = "A proxy to make a Javascript object behave like a Python object",
   .tp_methods = JsProxy_Methods,
   .tp_getset = JsProxy_GetSet,
@@ -585,8 +610,10 @@ JsProxy_cnew(JsRef idobj)
 {
   JsProxy* self;
   self = (JsProxy*)JsProxyType.tp_alloc(&JsProxyType, 0);
+  self->vectorcall = JsProxy_Vectorcall;
   self->js = hiwire_incref(idobj);
   self->bytes = NULL;
+  self->supports_kwargs = -1; // don't know
   self->awaited = false;
   return (PyObject*)self;
 }

src/pyodide.js

@@ -361,6 +361,95 @@ var languagePluginLoader = new Promise((resolve, reject) => {
     return Module.runPython(code);
   };
 
+  Module.function_supports_kwargs = function(funcstr) {
+    // This is basically a finite state machine (except for paren counting)
+    // Start at beginning of argspec
+    let idx = funcstr.indexOf("(") + 1;
+    // States:
+    // START_ARG -- Start of an argument. We leave this state when we see a non
+    // whitespace character.
+    //    If the first nonwhitespace character we see is `{` this is an object
+    //    destructuring argument. Else it's not. When we see non whitespace goto
+    //    state ARG and set `arg_is_obj_dest` true if it's "{", else false.
+    // ARG -- we're in the middle of an argument. Count parens. On comma, if
+    // parens_depth === 0 goto state START_ARG, on quote set
+    //      set quote_start and goto state QUOTE.
+    // QUOTE -- We're in a quote. Record quote_start in quote_start and look for
+    // a matching end quote.
+    //    On end quote, goto state ARGS. If we see "\\" goto state QUOTE_ESCAPE.
+    // QUOTE_ESCAPE -- unconditionally goto state QUOTE.
+    // If we see a ) when parens_depth === 0, return arg_is_obj_dest.
+    let START_ARG = 1;
+    let ARG = 2;
+    let QUOTE = 3;
+    let QUOTE_ESCAPE = 4;
+    let paren_depth = 0;
+    let arg_start = 0;
+    let arg_is_obj_dest = false;
+    let quote_start = undefined;
+    let state = START_ARG;
+    // clang-format off
+    for (i = idx; i < funcstr.length; i++) {
+      let x = funcstr[i];
+      if(state === QUOTE){
+        switch(x){
+          case quote_start:
+            // found match, go back to ARG
+            state = ARG;
+            continue;
+          case "\\":
+            state = QUOTE_ESCAPE;
+            continue;
+          default:
+            continue;
+        }
+      }
+      if(state === QUOTE_ESCAPE){
+        state = QUOTE;
+        continue;
+      }
+      // Skip whitespace.
+      if(x === " " || x === "\n" || x === "\t"){
+        continue;
+      }
+      if(paren_depth === 0){
+        if(x === ")" && state !== QUOTE && state !== QUOTE_ESCAPE){
+          // We hit closing brace which ends argspec.
+          // We have to handle this up here in case argspec ends in a trailing comma
+          // (if we're in state START_ARG, the next check would clobber arg_is_obj_dest).
+          return arg_is_obj_dest;
+        }
+        if(x === ","){
+          state = START_ARG;
+          continue;
+        }
+        // otherwise fall through
+      }
+      if(state === START_ARG){
+        // Nonwhitespace character in START_ARG so now we're in state arg.
+        state = ARG;
+        arg_is_obj_dest = x === "{";
+        // don't continue, fall through to next switch
+      }
+      switch(x){
+        case "[": case "{": case "(":
+          paren_depth ++;
+          continue;
+        case "]": case "}": case ")":
+          paren_depth--;
+          continue;
+        case "'": case '"': case '\`':
+          state = QUOTE;
+          quote_start = x;
+          continue;
+      }
+    }
+    // Correct exit is paren_depth === 0 && x === ")" test above.
+    throw new Error("Assertion failure: this is a logic error in \
+                     hiwire_function_supports_kwargs");
+    // clang-format on
+  };
+
   Module.locateFile = (path) => baseURL + path;
   Module.postRun = async () => {
     delete self.Module;

src/tests/test_jsproxy.py

@@ -173,25 +173,135 @@ def test_jsproxy_implicit_iter(selenium):
     ) == [1, 2, 3]
 
 
-def test_jsproxy_kwargs(selenium):
-    selenium.run_js(
-        """
-        window.kwarg_function = ({ a = 1, b = 1 }) => {
-            return a / b;
-        };
+def test_jsproxy_call(selenium):
+    assert (
+        selenium.run_js(
+            """
+        window.f = function(){ return arguments.length; };
+        return pyodide.runPython(
+            `
+            from js import f
+            [f(*range(n)) for n in range(10)]
+            `
+        );
         """
+        )
+        == list(range(10))
     )
+
+
+def test_jsproxy_call_kwargs(selenium):
     assert (
-        selenium.run(
+        selenium.run_js(
             """
-        from js import kwarg_function
-        kwarg_function(b = 2, a = 10)
+        window.kwarg_function = ({ a = 1, b = 1 }) => {
+            return [a, b];
+        };
+        return pyodide.runPython(
+            `
+            from js import kwarg_function
+            kwarg_function(b = 2, a = 10)
+            `
+        );
         """
         )
-        == 5
+        == [10, 2]
     )
 
 
+@pytest.mark.xfail
+def test_jsproxy_call_meth_py(selenium):
+    assert selenium.run_js(
+        """
+        window.a = {};
+        return pyodide.runPython(
+            `
+            from js import a
+            def f(self):
+                return self
+            a.f = f
+            a.f() == a
+            `
+        );
+        """
+    )
+
+
+def test_jsproxy_call_meth_js(selenium):
+    assert selenium.run_js(
+        """
+        window.a = {};
+        function f(){return this;}
+        a.f = f;
+        return pyodide.runPython(
+            `
+            from js import a
+            a.f() == a
+            `
+        );
+        """
+    )
+
+
+@pytest.mark.xfail
+def test_jsproxy_call_meth_js_kwargs(selenium):
+    assert selenium.run_js(
+        """
+        window.a = {};
+        function f({ x = 1, y = 1 }){
+            return [this, x, y];
+        }
+        a.f = f;
+        return pyodide.runPython(
+            `
+            from js import a
+            a.f(y=10, x=2) == [a, x, y]
+            `
+        );
+        """
+    )
+
+
+def test_supports_kwargs(selenium):
+    tests = [
+        ["", False],
+        ["x", False],
+        ["x     ", False],
+        ["{x}", True],
+        ["x, y, z", False],
+        ["x, y, {z}", True],
+        ["x, {y}, z", False],
+        ["x, {y}, {z}", True],
+        ["{}", True],
+        ["{} = {}", True],
+        ["[] = {}", False],
+        ["{} = []", True],
+        ["[] = []", False],
+        ["{} = null", True],
+        ["x = '2, `, {y}'", False],
+        ["{x} = '2, \\', x'", True],
+        ["[{x}]", False],
+        ["[x, y, z]", False],
+        ["x,", False],
+        ["{x},", True],
+        ["x, { y = 2 }", True],
+        ["{ y = 2 }, x", False],
+        ["{ x = 2 }, { y = 2 }", True],
+        ["{ a = 7, b = 2}", True],
+        ["{ a = 7, b = 2} = {b : 3}", True],
+        ["{ a = [7, 1], b = { c : 2} } = {}", True],
+        ["{ a = 7, b = 2} = {}", True],
+        ["{ a = 7, b = 2} = null", True],
+        ["{ x = { y : 2 }}", True],
+        ["{ x : 2 }", True],
+    ]
+    for (s, res) in tests:
+        s = f"function f({s}){{}}"
+        selenium.run_js(
+            f"return pyodide._module.function_supports_kwargs({repr(s)})"
+        ) == res
+
+
 import time
 
 ASYNCIO_EVENT_LOOP_STARTUP = """