-
-
Notifications
You must be signed in to change notification settings - Fork 863
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
Conversation
… before passing kwargs objects
@rth This is ready for review. |
I don't have much review bandwidth this week so I may take a while to respond/review (particularly for more complex PRs) |
Okay, thanks for letting me know. |
Just for fun, I posted |
Please add a changelog entry and maybe update the title to be more specific and mention kwargs. |
This resolves #974. There is a serious bug in
JsProxy_Call
:"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 usuallyNULL
(to avoid the overhead for making an empty dict).The definition of PyDict_Size is:
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 isjs2python
==>JsProxy_cnew
==>JsProxyType.tp_alloc
(neitherjs2python
norJsProxy_cnew
attempt any validation on their arguments). Chasing throughtp_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 theBadInternalCall
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.
Just typing
x.
into the browser console, the browser attempts to lookup the fields onx
for intellisense and callsdir
on the null pointer, so as I'm typing these examples the console is belching errors like this: