[PATCH v2 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> --- Note that native GetDispID seem to fail when retrieving builtin/prototype functions (?) if script is uninitialized, but retrieving the DISPID before and using it after it's uninitialized still works, though (that's what the tests do). I don't think it's worth replicating this GetDispID quirk for the moment. dlls/jscript/dispex.c | 5 +++ dlls/jscript/function.c | 10 ++--- dlls/jscript/tests/run.c | 86 ++++++++++++++++++++++++++++++++++++---- 3 files changed, 88 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..ae72f39 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 == E_UNEXPECTED, "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,14 +3215,62 @@ 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)); + ok(V_I4(&v) == 6, "V_I4(v) = %ld\n", V_I4(&v)); + + IDispatchEx_Release(dispex); + IActiveScript_Release(script); + + hres = parse_script_expr(L"Math.max", &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"call"); + hres = IDispatchEx_GetDispID(dispex, str, 0, &func_id); + SysFreeString(str); + ok(hres == S_OK, "GetDispID failed: %08lx\n", hres); + + str = SysAllocString(L"length"); + hres = IDispatchEx_GetDispID(dispex, str, 0, &prop_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, func_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)); + VariantClear(&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)); + ok(V_I4(&v) == 2, "V_I4(v) = %ld\n", V_I4(&v)); + IDispatchEx_Release(dispex); IActiveScript_Release(script); -- 2.34.1
On 5/31/22 16:56, Gabriel Ivăncescu wrote:
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> ---
Note that native GetDispID seem to fail when retrieving builtin/prototype functions (?) if script is uninitialized, but retrieving the DISPID before and using it after it's uninitialized still works, though (that's what the tests do). I don't think it's worth replicating this GetDispID quirk for the moment.
I'm not really concerned about GetDispID quirks, but implication of your choice for the next patch. I think that releasing objects sooner is better, so it would be good to have a better argument for moving that. Your reasoning there is based on details of how you decided to not follow native here. More generally, creating function objects in response for property get is an implementation detail that supposed to be transparent. If it's causing problems elsewhere, I'd first look at making it more transparent. In this case, maybe we should create function objects sooner, in response to GetDispID. This would both match all tests and allow us to release function constructor sooner. Jacek
On 01/06/2022 03:07, Jacek Caban wrote:
On 5/31/22 16:56, Gabriel Ivăncescu wrote:
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> ---
Note that native GetDispID seem to fail when retrieving builtin/prototype functions (?) if script is uninitialized, but retrieving the DISPID before and using it after it's uninitialized still works, though (that's what the tests do). I don't think it's worth replicating this GetDispID quirk for the moment.
I'm not really concerned about GetDispID quirks, but implication of your choice for the next patch. I think that releasing objects sooner is better, so it would be good to have a better argument for moving that. Your reasoning there is based on details of how you decided to not follow native here.
More generally, creating function objects in response for property get is an implementation detail that supposed to be transparent. If it's causing problems elsewhere, I'd first look at making it more transparent. In this case, maybe we should create function objects sooner, in response to GetDispID. This would both match all tests and allow us to release function constructor sooner.
Jacek
Right, but how is releasing such objects not an implementation detail as well? The thing is, there's a very rare crash in FFXIV Launcher when logging in that happens when some proxy prototype tries to obtain the object prototype, which for some reason is NULL (suggesting uninitialized script). I haven't been able to investigate it in depth since I wasn't able to reproduce it since then. But it would be neatly solved by this as well without adding any complexity. What's such a big deal about releasing them later? It's not like scripts stay that long in the uninitialized state, nor does it complicate the code, and it's an implementation detail anyway. If the function object was created earlier, it would still use the same function object, so the end result is exactly the same. On the other hand, obtaining the function object on dispid seems like too much work for no real gain. On top of work, that would also lose all the current optimizations with builtins (which are only retrieved now on PROPERTYGET, which is pretty rare—most of the time they're just called directly, while looking up their DISPIDs happens all the time). I'm definitely not certain that's a good idea.
On 6/1/22 15:48, Gabriel Ivăncescu wrote:
On 01/06/2022 03:07, Jacek Caban wrote:
On 5/31/22 16:56, Gabriel Ivăncescu wrote:
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> ---
Note that native GetDispID seem to fail when retrieving builtin/prototype functions (?) if script is uninitialized, but retrieving the DISPID before and using it after it's uninitialized still works, though (that's what the tests do). I don't think it's worth replicating this GetDispID quirk for the moment.
I'm not really concerned about GetDispID quirks, but implication of your choice for the next patch. I think that releasing objects sooner is better, so it would be good to have a better argument for moving that. Your reasoning there is based on details of how you decided to not follow native here.
More generally, creating function objects in response for property get is an implementation detail that supposed to be transparent. If it's causing problems elsewhere, I'd first look at making it more transparent. In this case, maybe we should create function objects sooner, in response to GetDispID. This would both match all tests and allow us to release function constructor sooner.
Jacek
Right, but how is releasing such objects not an implementation detail as well?
It's an exposed behaviour, see the attached test. BTW, having tests like that may be nice to catch leaks and test GC in the future. Jacek
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). Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- The previous patch already has tests for that builtin PROPERTYGET thing (with the DISPID obtained before setting it to uninitialized). dlls/jscript/global.c | 1 + dlls/jscript/jscript.c | 27 +++++++++++------------ dlls/jscript/jscript.h | 49 ++++++++++++++++++++++++------------------ 3 files changed, 41 insertions(+), 36 deletions(-) diff --git a/dlls/jscript/global.c b/dlls/jscript/global.c index c0ed954..1fe68b2 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; + script_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..3189528 100644 --- a/dlls/jscript/jscript.c +++ b/dlls/jscript/jscript.c @@ -71,11 +71,23 @@ typedef struct { jsexcept_t ei; } JScriptError; +void script_globals_release(script_ctx_t *ctx) +{ + unsigned i; + for(i = 0; i < ARRAY_SIZE(ctx->global_objects); i++) { + if(ctx->global_objects[i]) { + jsdisp_release(ctx->global_objects[i]); + ctx->global_objects[i] = NULL; + } + } +} + void script_release(script_ctx_t *ctx) { if(--ctx->ref) return; + script_globals_release(ctx); jsval_release(ctx->acc); if(ctx->cc) release_cc(ctx->cc); @@ -483,21 +495,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..c5c9043 100644 --- a/dlls/jscript/jscript.h +++ b/dlls/jscript/jscript.h @@ -388,29 +388,36 @@ struct _script_ctx_t { DWORD last_match_length; jsdisp_t *global; - jsdisp_t *function_constr; - jsdisp_t *array_constr; - jsdisp_t *bool_constr; - jsdisp_t *date_constr; - jsdisp_t *enumerator_constr; - jsdisp_t *error_constr; - jsdisp_t *eval_error_constr; - jsdisp_t *range_error_constr; - jsdisp_t *reference_error_constr; - jsdisp_t *regexp_error_constr; - jsdisp_t *syntax_error_constr; - jsdisp_t *type_error_constr; - jsdisp_t *uri_error_constr; - jsdisp_t *number_constr; - jsdisp_t *object_constr; - jsdisp_t *object_prototype; - jsdisp_t *regexp_constr; - jsdisp_t *string_constr; - jsdisp_t *vbarray_constr; - jsdisp_t *map_prototype; - jsdisp_t *set_prototype; + union { + struct { + jsdisp_t *function_constr; + jsdisp_t *array_constr; + jsdisp_t *bool_constr; + jsdisp_t *date_constr; + jsdisp_t *enumerator_constr; + jsdisp_t *error_constr; + jsdisp_t *eval_error_constr; + jsdisp_t *range_error_constr; + jsdisp_t *reference_error_constr; + jsdisp_t *regexp_error_constr; + jsdisp_t *syntax_error_constr; + jsdisp_t *type_error_constr; + jsdisp_t *uri_error_constr; + jsdisp_t *number_constr; + jsdisp_t *object_constr; + jsdisp_t *object_prototype; + jsdisp_t *regexp_constr; + jsdisp_t *string_constr; + jsdisp_t *vbarray_constr; + jsdisp_t *map_prototype; + jsdisp_t *set_prototype; + }; + jsdisp_t *global_objects[21]; + }; }; +C_ASSERT(RTL_SIZEOF_THROUGH_FIELD(script_ctx_t, set_prototype) == RTL_SIZEOF_THROUGH_FIELD(script_ctx_t, global_objects)); +void script_globals_release(script_ctx_t *ctx) DECLSPEC_HIDDEN; void script_release(script_ctx_t*) DECLSPEC_HIDDEN; static inline void script_addref(script_ctx_t *ctx) -- 2.34.1
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban