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

Fix serious soundness error in JsProxy_Call kwargs handling #1033

Merged
merged 26 commits into from Jan 13, 2021

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Jan 3, 2021

This resolves #974. There is a serious bug in JsProxy_Call:

  if (PyDict_Size(kwargs)) {
    JsRef idkwargs = python2js(kwargs);
    hiwire_push_array(idargs, idkwargs);
    hiwire_decref(idkwargs);
  }

"If there are no kwargs, the argument kwargs can be NULL or an empty dictionary." This code assumes it will be an empty dictionary but in practice, it's usually NULL (to avoid the overhead for making an empty dict).
The definition of PyDict_Size is:

Py_ssize_t
PyDict_Size(PyObject *mp)
{
    if (mp == NULL || !PyDict_Check(mp)) {
        PyErr_BadInternalCall();
        return -1;
    }
    return ((PyDictObject *)mp)->ma_used;
}

So we see that what usually happens is that it sets BadInternalCall and returns -1 indicating an error. As usual for this codebase, the calling code doesn't check for errors. What happens next is confusing to me. I think the next python API that is called is js2python ==> JsProxy_cnew ==> JsProxyType.tp_alloc (neither js2python nor JsProxy_cnew attempt any validation on their arguments). Chasing through tp_alloc to find where error handling happens is confusing, but on the master build, the error gets cleared. In various of my branches, I have added better error handling which causes the BadInternalCall to be correctly thrown.

The result of all of this is that most of the time javascript functions get called with an extra glitched argument.

function f(x){
    window.x = x;
}
pyodide.runPython("from js import f; f()");
x.toString() // ==>  "<NULL>"  (this is PyObject_Print(NULL, stdout, 0);)
x() // ==> Uncaught Error: TypeError: '�' object is not callable

Just typing x. into the browser console, the browser attempts to lookup the fields on x for intellisense and calls dir on the null pointer, so as I'm typing these examples the console is belching errors like this:

Python exception: pyodide.asm.js:8:504291
SystemError: frame does not exist

@hoodmane hoodmane changed the title Fix JsProxy_Call Fix serious soundness bug in JsProxy_Call Jan 3, 2021
@hoodmane hoodmane changed the title Fix serious soundness bug in JsProxy_Call Fix serious soundness error in JsProxy_Call Jan 3, 2021
@hoodmane
Copy link
Member Author

hoodmane commented Jan 6, 2021

@rth This is ready for review.

@rth
Copy link
Member

rth commented Jan 6, 2021

I don't have much review bandwidth this week so I may take a while to respond/review (particularly for more complex PRs)

@hoodmane
Copy link
Member Author

hoodmane commented Jan 6, 2021

Okay, thanks for letting me know.

@hoodmane hoodmane mentioned this pull request Jan 9, 2021
@hoodmane
Copy link
Member Author

@dalcde @rth I would much appreciate a review of this if either of you have time.

@hoodmane
Copy link
Member Author

Just for fun, I posted supports_kwargs as a codegolf question:
https://codegolf.stackexchange.com/questions/217511/does-this-javascript-function-support-keyword-arguments

src/core/jsproxy.c Show resolved Hide resolved
src/pyodide.js Show resolved Hide resolved
@rth
Copy link
Member

rth commented Jan 13, 2021

Please add a changelog entry and maybe update the title to be more specific and mention kwargs.

@hoodmane hoodmane changed the title Fix serious soundness error in JsProxy_Call Fix serious soundness error in JsProxy_Call kwargs handling Jan 13, 2021
@rth rth merged commit 7c0705c into pyodide:master Jan 13, 2021
@hoodmane hoodmane deleted the fix-jsproxy-call branch January 14, 2021 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: JsProxy.__call__ calls the js function with an extra argument
3 participants