[PATCH 1/3] jscript: Don't allow starting script using state from uninitialized.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/jscript.c | 2 +- dlls/jscript/tests/jscript.c | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/dlls/jscript/jscript.c b/dlls/jscript/jscript.c index 8cf141d..d6028e6 100644 --- a/dlls/jscript/jscript.c +++ b/dlls/jscript/jscript.c @@ -824,7 +824,7 @@ static HRESULT WINAPI JScript_SetScriptState(IActiveScript *iface, SCRIPTSTATE s switch(ss) { case SCRIPTSTATE_STARTED: case SCRIPTSTATE_CONNECTED: /* FIXME */ - if(This->ctx->state == SCRIPTSTATE_CLOSED) + if(This->ctx->state == SCRIPTSTATE_UNINITIALIZED || This->ctx->state == SCRIPTSTATE_CLOSED) return E_UNEXPECTED; exec_queued_code(This); diff --git a/dlls/jscript/tests/jscript.c b/dlls/jscript/tests/jscript.c index 8c985e5..8723541 100644 --- a/dlls/jscript/tests/jscript.c +++ b/dlls/jscript/tests/jscript.c @@ -863,6 +863,12 @@ static void test_jscript_uninitializing(void) hres = IActiveScript_SetScriptState(script, SCRIPTSTATE_UNINITIALIZED); ok(hres == S_OK, "SetScriptState(SCRIPTSTATE_UNINITIALIZED) failed: %08lx\n", hres); + hres = IActiveScript_SetScriptState(script, SCRIPTSTATE_STARTED); + ok(hres == E_UNEXPECTED, "SetScriptState(SCRIPTSTATE_STARTED) returned: %08lx\n", hres); + + hres = IActiveScript_SetScriptState(script, SCRIPTSTATE_CONNECTED); + ok(hres == E_UNEXPECTED, "SetScriptState(SCRIPTSTATE_CONNECTED) returned: %08lx\n", hres); + SET_EXPECT(GetLCID); SET_EXPECT(OnStateChange_INITIALIZED); hres = IActiveScript_SetScriptSite(script, &ActiveScriptSite); -- 2.34.1
Instead of only interpreted functions. Property retrievals or setters are allowed though, as long as they are not accessors. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/dispex.c | 5 +++ dlls/jscript/function.c | 10 ++--- dlls/jscript/tests/run.c | 90 ++++++++++++++++++++++++++++++++++++---- 3 files changed, 92 insertions(+), 13 deletions(-) diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c index 468a0dd..7980047 100644 --- a/dlls/jscript/dispex.c +++ b/dlls/jscript/dispex.c @@ -567,6 +567,8 @@ static HRESULT invoke_prop_func(jsdisp_t *This, IDispatch *jsthis, dispex_prop_t WARN("%s is not a constructor\n", debugstr_w(prop->name)); return E_INVALIDARG; } + if(This->ctx->state == SCRIPTSTATE_UNINITIALIZED || This->ctx->state == SCRIPTSTATE_CLOSED) + return E_UNEXPECTED; if(This->builtin_info->class != JSCLASS_FUNCTION && prop->u.p->invoke != JSGlobal_eval) flags &= ~DISPATCH_JSCRIPT_INTERNAL_MASK; @@ -1994,6 +1996,9 @@ HRESULT jsdisp_call_value(jsdisp_t *jsfunc, IDispatch *jsthis, WORD flags, unsig return JS_E_FUNCTION_EXPECTED; } + if(jsfunc->ctx->state == SCRIPTSTATE_UNINITIALIZED || jsfunc->ctx->state == SCRIPTSTATE_CLOSED) + return E_UNEXPECTED; + flags &= ~DISPATCH_JSCRIPT_INTERNAL_MASK; hres = jsfunc->builtin_info->call(jsfunc->ctx, jsthis ? jsval_disp(jsthis) : jsval_null(), flags, argc, argv, r); } diff --git a/dlls/jscript/function.c b/dlls/jscript/function.c index 638d176..a77ed29 100644 --- a/dlls/jscript/function.c +++ b/dlls/jscript/function.c @@ -257,6 +257,11 @@ HRESULT Function_invoke(jsdisp_t *func_this, IDispatch *jsthis, WORD flags, unsi assert(is_class(func_this, JSCLASS_FUNCTION)); function = function_from_jsdisp(func_this); + if(function->dispex.ctx->state == SCRIPTSTATE_UNINITIALIZED || function->dispex.ctx->state == SCRIPTSTATE_CLOSED) { + WARN("Script engine state does not allow running code.\n"); + return E_UNEXPECTED; + } + if(jsthis) vthis = jsval_disp(jsthis); else @@ -722,11 +727,6 @@ static HRESULT InterpretedFunction_call(script_ctx_t *ctx, FunctionInstance *fun TRACE("%p\n", function); - if(ctx->state == SCRIPTSTATE_UNINITIALIZED || ctx->state == SCRIPTSTATE_CLOSED) { - WARN("Script engine state does not allow running code.\n"); - return E_UNEXPECTED; - } - if(flags & DISPATCH_CONSTRUCT) { hres = create_object(ctx, &function->function.dispex, &new_obj); if(FAILED(hres)) diff --git a/dlls/jscript/tests/run.c b/dlls/jscript/tests/run.c index 94dc2e1..7656167 100644 --- a/dlls/jscript/tests/run.c +++ b/dlls/jscript/tests/run.c @@ -2914,13 +2914,20 @@ static void test_default_value(void) V_VT(&v) = VT_EMPTY; hres = IDispatch_Invoke(disp, DISPID_VALUE, &IID_NULL, 0, DISPATCH_PROPERTYGET, &dp, &v, NULL, NULL); - ok(hres == S_OK || broken(hres == 0x8000ffff), "Invoke failed: %08lx\n", hres); - if(hres == S_OK) - { - ok(V_VT(&v) == VT_BSTR, "V_VT(v) = %d\n", V_VT(&v)); - } + ok(hres == 0x8000ffff, "Invoke failed: %08lx\n", hres); + + hres = parse_script_expr(L"new Date()", &v, &script); + ok(hres == S_OK, "parse_script_expr failed: %08lx\n", hres); + ok(V_VT(&v) == VT_DISPATCH, "V_VT(v) = %d\n", V_VT(&v)); + disp = V_DISPATCH(&v); + + V_VT(&v) = VT_EMPTY; + hres = IDispatch_Invoke(disp, DISPID_VALUE, &IID_NULL, 0, DISPATCH_PROPERTYGET, &dp, &v, NULL, NULL); + ok(hres == S_OK, "Invoke failed: %08lx\n", hres); + ok(V_VT(&v) == VT_BSTR, "V_VT(v) = %d\n", V_VT(&v)); VariantClear(&v); IDispatch_Release(disp); + close_script(script); hres = parse_script_expr(L"var arr = [5]; arr.toString = function() {return \"foo\";}; arr.valueOf = function() {return 42;}; arr", &v, &script); ok(hres == S_OK, "parse_script_expr failed: %08lx\n", hres); @@ -3149,15 +3156,15 @@ static void test_script_exprs(void) static void test_invokeex(void) { - DISPID func_id, prop_id; - DISPPARAMS dp = {NULL}; + DISPPARAMS dp = {NULL}, dp_max = {NULL}; + DISPID func_id, max_id, prop_id; IActiveScript *script; IDispatchEx *dispex; VARIANT v, arg; BSTR str; HRESULT hres; - hres = parse_script_expr(L"var o = {func: function() {return 3;}, prop: 6}; o", &v, &script); + hres = parse_script_expr(L"var o = {func: function() {return 3;}, max: Math.max, prop: 6}; o", &v, &script); ok(hres == S_OK, "parse_script_expr failed: %08lx\n", hres); ok(V_VT(&v) == VT_DISPATCH, "V_VT(v) = %d\n", V_VT(&v)); @@ -3170,16 +3177,31 @@ static void test_invokeex(void) SysFreeString(str); ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); + str = SysAllocString(L"max"); + hres = IDispatchEx_GetDispID(dispex, str, 0, &max_id); + SysFreeString(str); + ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); + str = SysAllocString(L"prop"); hres = IDispatchEx_GetDispID(dispex, str, 0, &prop_id); SysFreeString(str); ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); + dp_max.rgvarg = &arg; + dp_max.cArgs = 1; + V_VT(&arg) = VT_I4; + V_I4(&arg) = 42; + hres = IDispatchEx_InvokeEx(dispex, func_id, 0, DISPATCH_METHOD, &dp, &v, NULL, NULL); ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); ok(V_VT(&v) == VT_I4, "V_VT(v) = %d\n", V_VT(&v)); ok(V_I4(&v) == 3, "V_I4(v) = %ld\n", V_I4(&v)); + hres = IDispatchEx_InvokeEx(dispex, max_id, 0, DISPATCH_METHOD, &dp_max, &v, NULL, NULL); + ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); + ok(V_VT(&v) == VT_I4, "V_VT(v) = %d\n", V_VT(&v)); + ok(V_I4(&v) == 42, "V_I4(v) = %ld\n", V_I4(&v)); + hres = IDispatchEx_InvokeEx(dispex, prop_id, 0, DISPATCH_PROPERTYGET, &dp, &v, NULL, NULL); ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); ok(V_VT(&v) == VT_I4, "V_VT(v) = %d\n", V_VT(&v)); @@ -3193,9 +3215,23 @@ static void test_invokeex(void) SysFreeString(str); ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); + V_VT(&v) = VT_EMPTY; hres = IDispatchEx_InvokeEx(dispex, func_id, 0, DISPATCH_METHOD, &dp, &v, NULL, NULL); ok(hres == E_UNEXPECTED || broken(hres == 0x800a1393), "InvokeEx failed: %08lx\n", hres); + V_VT(&v) = VT_EMPTY; + hres = IDispatchEx_InvokeEx(dispex, max_id, 0, DISPATCH_METHOD, &dp_max, &v, NULL, NULL); + ok(hres == E_UNEXPECTED || broken(hres == 0x800a1393), "InvokeEx failed: %08lx\n", hres); + + V_VT(&v) = VT_EMPTY; + hres = IDispatchEx_InvokeEx(dispex, prop_id, 0, DISPATCH_PROPERTYGET, &dp, &v, NULL, NULL); + ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); + ok(V_VT(&v) == VT_I4, "V_VT(v) = %d\n", V_VT(&v)); + ok(V_I4(&v) == 6, "V_I4(v) = %ld\n", V_I4(&v)); + + IActiveScript_Close(script); + + V_VT(&v) = VT_EMPTY; hres = IDispatchEx_InvokeEx(dispex, prop_id, 0, DISPATCH_PROPERTYGET, &dp, &v, NULL, NULL); ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); ok(V_VT(&v) == VT_I4, "V_VT(v) = %d\n", V_VT(&v)); @@ -3204,6 +3240,44 @@ static void test_invokeex(void) IDispatchEx_Release(dispex); IActiveScript_Release(script); + hres = parse_script_expr(L"Math", &v, &script); + ok(hres == S_OK, "parse_script_expr failed: %08lx\n", hres); + ok(V_VT(&v) == VT_DISPATCH, "V_VT(v) = %d\n", V_VT(&v)); + + hres = IDispatch_QueryInterface(V_DISPATCH(&v), &IID_IDispatchEx, (void**)&dispex); + ok(hres == S_OK, "Could not get IDispatchEx iface: %08lx\n", hres); + VariantClear(&v); + + str = SysAllocString(L"max"); + hres = IDispatchEx_GetDispID(dispex, str, 0, &max_id); + SysFreeString(str); + ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); + + hres = IActiveScript_SetScriptState(script, SCRIPTSTATE_UNINITIALIZED); + ok(hres == S_OK, "SetScriptState(SCRIPTSTATE_STARTED) failed: %08lx\n", hres); + + hres = IDispatchEx_InvokeEx(dispex, max_id, 0, DISPATCH_PROPERTYGET, &dp, &v, NULL, NULL); + ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); + ok(V_VT(&v) == VT_DISPATCH, "V_VT(v) = %d\n", V_VT(&v)); + + IDispatchEx_Release(dispex); + hres = IDispatch_QueryInterface(V_DISPATCH(&v), &IID_IDispatchEx, (void**)&dispex); + ok(hres == S_OK, "Could not get IDispatchEx iface: %08lx\n", hres); + VariantClear(&v); + + str = SysAllocString(L"length"); + hres = IDispatchEx_GetDispID(dispex, str, 0, &prop_id); + SysFreeString(str); + ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); + + hres = IDispatchEx_InvokeEx(dispex, prop_id, 0, DISPATCH_PROPERTYGET, &dp, &v, NULL, NULL); + ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); + ok(V_VT(&v) == VT_I4, "V_VT(v) = %d\n", V_VT(&v)); + ok(V_I4(&v) == 2, "V_I4(v) = %ld\n", V_I4(&v)); + + IDispatchEx_Release(dispex); + IActiveScript_Release(script); + /* test InvokeEx following prototype chain of builtin object (PROP_PROTREF) */ hres = parse_script_expr(L"o = new Array(); o.push(\"foo\"); o", &v, &script); ok(hres == S_OK, "parse_script_expr failed: %08lx\n", hres); -- 2.34.1
Most of these globals were leaking before as they were never freed at all. Also, they have to be freed during script ctx destruction because an unintialized script might still make use of them (e.g. retrieving a builtin function via PROPERTYGET requires ctx->function_constr to be available), so freeing them during state transition would crash. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- The previous patch already has tests for that builtin PROPERTYGET thing. dlls/jscript/global.c | 1 + dlls/jscript/jscript.c | 16 +--------------- dlls/jscript/jscript.h | 12 ++++++++++++ 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/dlls/jscript/global.c b/dlls/jscript/global.c index c0ed954..1852cd3 100644 --- a/dlls/jscript/global.c +++ b/dlls/jscript/global.c @@ -1085,6 +1085,7 @@ HRESULT init_global(script_ctx_t *ctx) if(ctx->global) return S_OK; + globals_release(ctx); hres = create_dispex(ctx, &JSGlobal_info, NULL, &ctx->global); if(FAILED(hres)) diff --git a/dlls/jscript/jscript.c b/dlls/jscript/jscript.c index d6028e6..01249c4 100644 --- a/dlls/jscript/jscript.c +++ b/dlls/jscript/jscript.c @@ -76,6 +76,7 @@ void script_release(script_ctx_t *ctx) if(--ctx->ref) return; + globals_release(ctx); jsval_release(ctx->acc); if(ctx->cc) release_cc(ctx->cc); @@ -483,21 +484,6 @@ static void decrease_state(JScript *This, SCRIPTSTATE state) This->ctx->site = NULL; } - if(This->ctx->map_prototype) { - jsdisp_release(This->ctx->map_prototype); - This->ctx->map_prototype = NULL; - } - - if(This->ctx->set_prototype) { - jsdisp_release(This->ctx->set_prototype); - This->ctx->set_prototype = NULL; - } - - if(This->ctx->object_prototype) { - jsdisp_release(This->ctx->object_prototype); - This->ctx->object_prototype = NULL; - } - if(This->ctx->global) { jsdisp_release(This->ctx->global); This->ctx->global = NULL; diff --git a/dlls/jscript/jscript.h b/dlls/jscript/jscript.h index 000bcc2..f635c09 100644 --- a/dlls/jscript/jscript.h +++ b/dlls/jscript/jscript.h @@ -411,6 +411,18 @@ struct _script_ctx_t { jsdisp_t *set_prototype; }; +static inline void globals_release(script_ctx_t *ctx) +{ + jsdisp_t **iter = &ctx->function_constr, **end = &ctx->set_prototype + 1; + while(iter != end) { + if(*iter) { + jsdisp_release(*iter); + *iter = NULL; + } + iter++; + } +} + void script_release(script_ctx_t*) DECLSPEC_HIDDEN; static inline void script_addref(script_ctx_t *ctx) -- 2.34.1
On 5/30/22 19:24, Gabriel Ivăncescu wrote:
Most of these globals were leaking before as they were never freed at all. Also, they have to be freed during script ctx destruction because an unintialized script might still make use of them (e.g. retrieving a builtin function via PROPERTYGET requires ctx->function_constr to be available), so freeing them during state transition would crash.
I checked it (see the attached patch) and in such case function prototype is not really functional on Windows. This means that ctx->function_constr is not really needed for them. I didn't test it further, but I wouldn't be surprised if on Windows, all objects would be "detached" at this point from both ctx and prototype.
+static inline void globals_release(script_ctx_t *ctx) +{ + jsdisp_t **iter = &ctx->function_constr, **end = &ctx->set_prototype + 1; + while(iter != end) { + if(*iter) { + jsdisp_release(*iter); + *iter = NULL; + } + iter++; + } +}
That's ugly. We could potentially store those in array in the first place if we really need something like this. Also, there is no need for inline. Thanks, Jacek
Hi Jacek, On 30/05/2022 21:18, Jacek Caban wrote:
On 5/30/22 19:24, Gabriel Ivăncescu wrote:
Most of these globals were leaking before as they were never freed at all. Also, they have to be freed during script ctx destruction because an unintialized script might still make use of them (e.g. retrieving a builtin function via PROPERTYGET requires ctx->function_constr to be available), so freeing them during state transition would crash.
I checked it (see the attached patch) and in such case function prototype is not really functional on Windows. This means that ctx->function_constr is not really needed for them. I didn't test it further, but I wouldn't be surprised if on Windows, all objects would be "detached" at this point from both ctx and prototype.
I can't test it right now, will do more tests tomorrow, but AFAIK GetDispID doesn't work on native, that's why I retrieve it before changing state (even for "length"), although obtaining it after via the same DISPID works. Can you also try to retrieve "call" before setting it to uninitialized and then using the DISPID? If it returns a VT_DISPATCH with PROPERTYGET it means it works I guess...
+static inline void globals_release(script_ctx_t *ctx) +{ + jsdisp_t **iter = &ctx->function_constr, **end = &ctx->set_prototype + 1; + while(iter != end) { + if(*iter) { + jsdisp_release(*iter); + *iter = NULL; + } + iter++; + } +}
That's ugly. We could potentially store those in array in the first place if we really need something like this. Also, there is no need for inline.
Thanks,
Jacek
I made it inline because I wanted to place it in the header right below the struct so it's kept in sync. I agree an array would be nicer for releasing them, but it would make the rest of the code a lot uglier IMO. How about a union of an array and a struct with current fields? (with a comment saying to keep its length in sync). That would at least keep the rest of the code the same, instead of stuff like: ctx->globals[CTX_OBJECT_PROTOTYPE]; which IMO is uglier.
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban