commit 983e8ec37b0ec1cc5114cb9ca49cf558dedfb31e Author: Miloslav Trmač Date: Tue Jul 1 20:00:48 2014 +0200 Don't pass an uninitialized JS parameter Don't pass argc==3 when using a 2-member array in polkit_backend_js_authority_check_authorization_sync . To avoid such problems in the future, use G_N_ELEMENTS in both similar callers. https://bugs.freedesktop.org/show_bug.cgi?id=69501 diff --git a/src/polkitbackend/polkitbackendjsauthority.c b/src/polkitbackend/polkitbackendjsauthority.c index c232573..c7a29e0 100644 --- a/src/polkitbackend/polkitbackendjsauthority.c +++ b/src/polkitbackend/polkitbackendjsauthority.c @@ -1074,7 +1074,7 @@ polkit_backend_js_authority_get_admin_auth_identities (PolkitBackendInteractiveA if (!call_js_function_with_runaway_killer (authority, "_runAdminRules", - 2, + G_N_ELEMENTS (argv), argv, &rval)) { @@ -1179,7 +1179,7 @@ polkit_backend_js_authority_check_authorization_sync (PolkitBackendInteractiveAu if (!call_js_function_with_runaway_killer (authority, "_runRules", - 3, + G_N_ELEMENTS (argv), argv, &rval)) { commit a97672540c66c03ed392fc072f0c682281f08989 Author: Miloslav Trmač Date: Tue Jul 1 20:00:48 2014 +0200 Don't add extra NULL group to subject.groups The NULL “terminator” of ‘groups’ was being passed to JavaScript. Drop it, and simplify by leting set_property_strv use the GPtrArray directly instead of the extra conversions “into” a strv and a completely dead g_strv_length(). https://bugs.freedesktop.org/show_bug.cgi?id=69501 diff --git a/src/polkitbackend/polkitbackendjsauthority.c b/src/polkitbackend/polkitbackendjsauthority.c index c7a29e0..efb07a9 100644 --- a/src/polkitbackend/polkitbackendjsauthority.c +++ b/src/polkitbackend/polkitbackendjsauthority.c @@ -659,26 +659,22 @@ static void set_property_strv (PolkitBackendJsAuthority *authority, JSObject *obj, const gchar *name, - const gchar *const *value, - gssize len) + GPtrArray *value) { jsval value_jsval; JSObject *array_object; jsval *jsvals; guint n; - if (len < 0) - len = g_strv_length ((gchar **) value); - - jsvals = g_new0 (jsval, len); - for (n = 0; n < len; n++) + jsvals = g_new0 (jsval, value->len); + for (n = 0; n < value->len; n++) { JSString *jsstr; - jsstr = JS_NewStringCopyZ (authority->priv->cx, value[n]); + jsstr = JS_NewStringCopyZ (authority->priv->cx, g_ptr_array_index(value, n)); jsvals[n] = STRING_TO_JSVAL (jsstr); } - array_object = JS_NewArrayObject (authority->priv->cx, (gint32) len, jsvals); + array_object = JS_NewArrayObject (authority->priv->cx, value->len, jsvals); value_jsval = OBJECT_TO_JSVAL (array_object); JS_SetProperty (authority->priv->cx, obj, name, &value_jsval); @@ -818,11 +814,9 @@ subject_to_jsval (PolkitBackendJsAuthority *authority, } } - g_ptr_array_add (groups, NULL); - set_property_int32 (authority, obj, "pid", pid); set_property_str (authority, obj, "user", user_name); - set_property_strv (authority, obj, "groups", (const gchar* const *) groups->pdata, groups->len); + set_property_strv (authority, obj, "groups", groups); set_property_str (authority, obj, "seat", seat_str); set_property_str (authority, obj, "session", session_str); set_property_bool (authority, obj, "local", subject_is_local); commit cbad0d5721804a4b7c2d998b00da9e70dc623820 Author: Miloslav Trmač Date: Tue Jul 1 20:00:48 2014 +0200 Don't store unrooted jsvals on heap Don't create a temporary array of jsvals on heap; the GC is not looking for GC roots there. Compare https://developer.mozilla.org/en-US/docs/SpiderMonkey/GC_Rooting_Guide and https://web.archive.org/web/20140305233124/https://developer.mozilla.org/en-US/docs/SpiderMonkey_Garbage_Collection_Tips . https://bugs.freedesktop.org/show_bug.cgi?id=69501 diff --git a/src/polkitbackend/polkitbackendjsauthority.c b/src/polkitbackend/polkitbackendjsauthority.c index efb07a9..d02e5e3 100644 --- a/src/polkitbackend/polkitbackendjsauthority.c +++ b/src/polkitbackend/polkitbackendjsauthority.c @@ -663,23 +663,22 @@ set_property_strv (PolkitBackendJsAuthority *authority, { jsval value_jsval; JSObject *array_object; - jsval *jsvals; guint n; - jsvals = g_new0 (jsval, value->len); + array_object = JS_NewArrayObject (authority->priv->cx, 0, NULL); + for (n = 0; n < value->len; n++) { JSString *jsstr; + jsval val; + jsstr = JS_NewStringCopyZ (authority->priv->cx, g_ptr_array_index(value, n)); - jsvals[n] = STRING_TO_JSVAL (jsstr); + val = STRING_TO_JSVAL (jsstr); + JS_SetElement (authority->priv->cx, array_object, n, &val); } - array_object = JS_NewArrayObject (authority->priv->cx, value->len, jsvals); - value_jsval = OBJECT_TO_JSVAL (array_object); JS_SetProperty (authority->priv->cx, obj, name, &value_jsval); - - g_free (jsvals); } commit 0f5852a4bdabe377ddcdbed09a0c1f95710e17fe Author: Miloslav Trmač Date: Tue Jul 1 20:00:48 2014 +0200 Fix a per-authorization memory leak We were leaking PolkitAuthorizationResult on every request, primarily on the success path, but also on various error paths as well. https://bugs.freedesktop.org/show_bug.cgi?id=69501 diff --git a/src/polkitbackend/polkitbackendauthority.c b/src/polkitbackend/polkitbackendauthority.c index a09d667..14eea99 100644 --- a/src/polkitbackend/polkitbackendauthority.c +++ b/src/polkitbackend/polkitbackendauthority.c @@ -714,6 +714,7 @@ check_auth_cb (GObject *source_object, g_variant_ref_sink (value); g_dbus_method_invocation_return_value (data->invocation, g_variant_new ("(@(bba{ss}))", value)); g_variant_unref (value); + g_object_unref (result); } check_auth_data_free (data); diff --git a/src/polkitbackend/polkitbackendinteractiveauthority.c b/src/polkitbackend/polkitbackendinteractiveauthority.c index 96725f7..7019356 100644 --- a/src/polkitbackend/polkitbackendinteractiveauthority.c +++ b/src/polkitbackend/polkitbackendinteractiveauthority.c @@ -1022,7 +1022,7 @@ polkit_backend_interactive_authority_check_authorization (PolkitBackendAuthority /* Otherwise just return the result */ g_simple_async_result_set_op_res_gpointer (simple, - result, + g_object_ref (result), g_object_unref); g_simple_async_result_complete (simple); g_object_unref (simple); @@ -1039,6 +1039,9 @@ polkit_backend_interactive_authority_check_authorization (PolkitBackendAuthority g_free (subject_str); g_free (user_of_caller_str); g_free (user_of_subject_str); + + if (result != NULL) + g_object_unref (result); } /* ---------------------------------------------------------------------------------------------------- */ commit ec039f9d7ede5b839f5511e26d5cd6ae9107cb2e Author: Miloslav Trmač Date: Tue Jul 1 20:00:48 2014 +0200 Fix a memory leak when registering an authentication agent https://bugs.freedesktop.org/show_bug.cgi?id=69501 diff --git a/src/polkitbackend/polkitbackendauthority.c b/src/polkitbackend/polkitbackendauthority.c index 14eea99..64560e1 100644 --- a/src/polkitbackend/polkitbackendauthority.c +++ b/src/polkitbackend/polkitbackendauthority.c @@ -900,6 +900,7 @@ server_handle_register_authentication_agent (Server *server, g_dbus_method_invocation_return_value (invocation, g_variant_new ("()")); out: + g_variant_unref (subject_gvariant); if (subject != NULL) g_object_unref (subject); } commit 57e2d86edc2630cac1812a3285715dad795a4bd6 Author: Miloslav Trmač Date: Tue Jul 1 20:00:48 2014 +0200 Wrap all JS usage within “requests” Required by https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_THREADSAFE ; lack of requests causes assertion failures with a debug build of mozjs17. https://bugs.freedesktop.org/show_bug.cgi?id=69501 diff --git a/src/polkitbackend/polkitbackendjsauthority.c b/src/polkitbackend/polkitbackendjsauthority.c index d02e5e3..88f31bd 100644 --- a/src/polkitbackend/polkitbackendjsauthority.c +++ b/src/polkitbackend/polkitbackendjsauthority.c @@ -239,6 +239,7 @@ rules_file_name_cmp (const gchar *a, return ret; } +/* authority->priv->cx must be within a request */ static void load_scripts (PolkitBackendJsAuthority *authority) { @@ -339,6 +340,8 @@ reload_scripts (PolkitBackendJsAuthority *authority) jsval argv[1] = {JSVAL_NULL}; jsval rval = JSVAL_NULL; + JS_BeginRequest (authority->priv->cx); + if (!JS_CallFunctionName(authority->priv->cx, authority->priv->js_polkit, "_deleteRules", @@ -364,7 +367,7 @@ reload_scripts (PolkitBackendJsAuthority *authority) /* Let applications know we have new rules... */ g_signal_emit_by_name (authority, "changed"); out: - ; + JS_EndRequest (authority->priv->cx); } static void @@ -447,6 +450,7 @@ static void polkit_backend_js_authority_constructed (GObject *object) { PolkitBackendJsAuthority *authority = POLKIT_BACKEND_JS_AUTHORITY (object); + gboolean entered_request = FALSE; authority->priv->rt = JS_NewRuntime (8L * 1024L * 1024L); if (authority->priv->rt == NULL) @@ -466,6 +470,9 @@ polkit_backend_js_authority_constructed (GObject *object) JS_SetErrorReporter(authority->priv->cx, report_error); JS_SetContextPrivate (authority->priv->cx, authority); + JS_BeginRequest(authority->priv->cx); + entered_request = TRUE; + authority->priv->js_global = #if JS_VERSION == 186 JS_NewGlobalObject (authority->priv->cx, &js_global_class, NULL); @@ -526,10 +533,15 @@ polkit_backend_js_authority_constructed (GObject *object) setup_file_monitors (authority); load_scripts (authority); + JS_EndRequest (authority->priv->cx); + entered_request = FALSE; + G_OBJECT_CLASS (polkit_backend_js_authority_parent_class)->constructed (object); return; fail: + if (entered_request) + JS_EndRequest (authority->priv->cx); g_critical ("Error initializing JavaScript environment"); g_assert_not_reached (); } @@ -642,6 +654,7 @@ polkit_backend_js_authority_class_init (PolkitBackendJsAuthorityClass *klass) /* ---------------------------------------------------------------------------------------------------- */ +/* authority->priv->cx must be within a request */ static void set_property_str (PolkitBackendJsAuthority *authority, JSObject *obj, @@ -655,6 +668,7 @@ set_property_str (PolkitBackendJsAuthority *authority, JS_SetProperty (authority->priv->cx, obj, name, &value_jsval); } +/* authority->priv->cx must be within a request */ static void set_property_strv (PolkitBackendJsAuthority *authority, JSObject *obj, @@ -681,7 +695,7 @@ set_property_strv (PolkitBackendJsAuthority *authority, JS_SetProperty (authority->priv->cx, obj, name, &value_jsval); } - +/* authority->priv->cx must be within a request */ static void set_property_int32 (PolkitBackendJsAuthority *authority, JSObject *obj, @@ -693,6 +707,7 @@ set_property_int32 (PolkitBackendJsAuthority *authority, JS_SetProperty (authority->priv->cx, obj, name, &value_jsval); } +/* authority->priv->cx must be within a request */ static void set_property_bool (PolkitBackendJsAuthority *authority, JSObject *obj, @@ -706,6 +721,7 @@ set_property_bool (PolkitBackendJsAuthority *authority, /* ---------------------------------------------------------------------------------------------------- */ +/* authority->priv->cx must be within a request */ static gboolean subject_to_jsval (PolkitBackendJsAuthority *authority, PolkitSubject *subject, @@ -838,6 +854,7 @@ subject_to_jsval (PolkitBackendJsAuthority *authority, /* ---------------------------------------------------------------------------------------------------- */ +/* authority->priv->cx must be within a request */ static gboolean action_and_details_to_jsval (PolkitBackendJsAuthority *authority, const gchar *action_id, @@ -1041,6 +1058,8 @@ polkit_backend_js_authority_get_admin_auth_identities (PolkitBackendInteractiveA gchar *ret_str = NULL; gchar **ret_strs = NULL; + JS_BeginRequest (authority->priv->cx); + if (!action_and_details_to_jsval (authority, action_id, details, &argv[0], &error)) { polkit_backend_authority_log (POLKIT_BACKEND_AUTHORITY (authority), @@ -1120,6 +1139,8 @@ polkit_backend_js_authority_get_admin_auth_identities (PolkitBackendInteractiveA JS_MaybeGC (authority->priv->cx); + JS_EndRequest (authority->priv->cx); + return ret; } @@ -1146,6 +1167,8 @@ polkit_backend_js_authority_check_authorization_sync (PolkitBackendInteractiveAu gchar *ret_str = NULL; gboolean good = FALSE; + JS_BeginRequest (authority->priv->cx); + if (!action_and_details_to_jsval (authority, action_id, details, &argv[0], &error)) { polkit_backend_authority_log (POLKIT_BACKEND_AUTHORITY (authority), @@ -1222,6 +1245,8 @@ polkit_backend_js_authority_check_authorization_sync (PolkitBackendInteractiveAu JS_MaybeGC (authority->priv->cx); + JS_EndRequest (authority->priv->cx); + return ret; } commit 5c668722320eb363f713a0998934aa48fecd56cb Author: Miloslav Trmač Date: Tue Jul 1 20:00:48 2014 +0200 Register heap-based JSObject pointers to GC This is necessary so that the GC can move the objects (though I haven't so far encountered this in testing). https://bugs.freedesktop.org/show_bug.cgi?id=69501 diff --git a/src/polkitbackend/polkitbackendjsauthority.c b/src/polkitbackend/polkitbackendjsauthority.c index 88f31bd..39f7060 100644 --- a/src/polkitbackend/polkitbackendjsauthority.c +++ b/src/polkitbackend/polkitbackendjsauthority.c @@ -482,6 +482,7 @@ polkit_backend_js_authority_constructed (GObject *object) if (authority->priv->js_global == NULL) goto fail; + JS_AddObjectRoot (authority->priv->cx, &authority->priv->js_global); if (!JS_InitStandardClasses (authority->priv->cx, authority->priv->js_global)) goto fail; @@ -494,6 +495,7 @@ polkit_backend_js_authority_constructed (GObject *object) JSPROP_ENUMERATE); if (authority->priv->js_polkit == NULL) goto fail; + JS_AddObjectRoot (authority->priv->cx, &authority->priv->js_polkit); if (!JS_DefineFunctions (authority->priv->cx, authority->priv->js_polkit, @@ -572,6 +574,11 @@ polkit_backend_js_authority_finalize (GObject *object) g_free (authority->priv->dir_monitors); g_strfreev (authority->priv->rules_dirs); + JS_BeginRequest (authority->priv->cx); + JS_RemoveObjectRoot (authority->priv->cx, &authority->priv->js_polkit); + JS_RemoveObjectRoot (authority->priv->cx, &authority->priv->js_global); + JS_EndRequest (authority->priv->cx); + JS_DestroyContext (authority->priv->cx); JS_DestroyRuntime (authority->priv->rt); /* JS_ShutDown (); */ commit 2881f8b260c03df29afb0e35e6d1707240f95ad7 Author: Miloslav Trmač Date: Tue Jul 1 20:00:48 2014 +0200 Prevent builds against SpiderMonkey with exact stack rooting “Exact stack rooting” means that every on-stack pointer to a JavaScript value needs to be registered with the runtime. The current code doesn't do this, so it is not safe to use against a runtime with this configuration. Luckily this configuration is not default. See https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/GC/Exact_Stack_Rooting and other pages in the wiki for what the conversion would require. https://bugs.freedesktop.org/show_bug.cgi?id=69501 diff --git a/src/polkitbackend/polkitbackendjsauthority.c b/src/polkitbackend/polkitbackendjsauthority.c index 39f7060..22812a6 100644 --- a/src/polkitbackend/polkitbackendjsauthority.c +++ b/src/polkitbackend/polkitbackendjsauthority.c @@ -43,6 +43,13 @@ #include "initjs.h" /* init.js */ +#ifdef JSGC_USE_EXACT_ROOTING +/* See https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/GC/Exact_Stack_Rooting + * for more information about exact stack rooting. + */ +#error "This code is not safe in SpiderMonkey exact stack rooting configurations" +#endif + /** * SECTION:polkitbackendjsauthority * @title: PolkitBackendJsAuthority commit b544f10dd469ae3cfedc026db71ee76e9ef511a2 Author: Miloslav Trmač Date: Tue Jul 1 20:00:48 2014 +0200 Clear the JS operation callback before invoking JS in the callback Setting the callback to NULL is required by https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JS_SetOperationCallback to avoid the possibility of recursion. https://bugs.freedesktop.org/show_bug.cgi?id=69501 diff --git a/src/polkitbackend/polkitbackendjsauthority.c b/src/polkitbackend/polkitbackendjsauthority.c index 22812a6..8a0a097 100644 --- a/src/polkitbackend/polkitbackendjsauthority.c +++ b/src/polkitbackend/polkitbackendjsauthority.c @@ -961,9 +961,11 @@ js_operation_callback (JSContext *cx) polkit_backend_authority_log (POLKIT_BACKEND_AUTHORITY (authority), "Terminating runaway script"); /* Throw an exception - this way the JS code can ignore the runaway script handling */ + JS_SetOperationCallback (authority->priv->cx, NULL); val_str = JS_NewStringCopyZ (cx, "Terminating runaway script"); val = STRING_TO_JSVAL (val_str); JS_SetPendingException (authority->priv->cx, val); + JS_SetOperationCallback (authority->priv->cx, js_operation_callback); return JS_FALSE; } commit d7da6a23766e9c95fa333a0a9c742f7397c0ad22 Author: Miloslav Trmač Date: Tue Jul 1 20:00:48 2014 +0200 Fix spurious timeout exceptions on GC The JS “Operation callback” can be called by the runtime for other reasons, not only when we trigger it by a timeout—notably as part of GC. So, make sure to only raise an exception if there actually was a timeout. Adding a whole extra mutex to protect a single boolean is somewhat of an overkill, but better than worrying about “subtle bugs and occasionally undefined behaviour” the g_atomic_* API is warning about. https://bugs.freedesktop.org/show_bug.cgi?id=69501 also https://bugs.freedesktop.org/show_bug.cgi?id=77524 diff --git a/src/polkitbackend/polkitbackendjsauthority.c b/src/polkitbackend/polkitbackendjsauthority.c index 8a0a097..097dcc5 100644 --- a/src/polkitbackend/polkitbackendjsauthority.c +++ b/src/polkitbackend/polkitbackendjsauthority.c @@ -80,6 +80,8 @@ struct _PolkitBackendJsAuthorityPrivate GMainContext *rkt_context; GMainLoop *rkt_loop; GSource *rkt_source; + GMutex rkt_timeout_pending_mutex; + gboolean rkt_timeout_pending; /* A list of JSObject instances */ GList *scripts; @@ -528,6 +530,7 @@ polkit_backend_js_authority_constructed (GObject *object) g_mutex_init (&authority->priv->rkt_init_mutex); g_cond_init (&authority->priv->rkt_init_cond); + g_mutex_init (&authority->priv->rkt_timeout_pending_mutex); authority->priv->runaway_killer_thread = g_thread_new ("runaway-killer-thread", runaway_killer_thread_func, @@ -563,6 +566,7 @@ polkit_backend_js_authority_finalize (GObject *object) g_mutex_clear (&authority->priv->rkt_init_mutex); g_cond_clear (&authority->priv->rkt_init_cond); + g_mutex_clear (&authority->priv->rkt_timeout_pending_mutex); /* shut down the killer thread */ g_assert (authority->priv->rkt_loop != NULL); @@ -957,6 +961,18 @@ js_operation_callback (JSContext *cx) JSString *val_str; jsval val; + /* This callback can be called by the runtime at any time without us causing + * it by JS_TriggerOperationCallback(). + */ + g_mutex_lock (&authority->priv->rkt_timeout_pending_mutex); + if (!authority->priv->rkt_timeout_pending) + { + g_mutex_unlock (&authority->priv->rkt_timeout_pending_mutex); + return JS_TRUE; + } + authority->priv->rkt_timeout_pending = FALSE; + g_mutex_unlock (&authority->priv->rkt_timeout_pending_mutex); + /* Log that we are terminating the script */ polkit_backend_authority_log (POLKIT_BACKEND_AUTHORITY (authority), "Terminating runaway script"); @@ -974,6 +990,10 @@ rkt_on_timeout (gpointer user_data) { PolkitBackendJsAuthority *authority = POLKIT_BACKEND_JS_AUTHORITY (user_data); + g_mutex_lock (&authority->priv->rkt_timeout_pending_mutex); + authority->priv->rkt_timeout_pending = TRUE; + g_mutex_unlock (&authority->priv->rkt_timeout_pending_mutex); + /* Supposedly this is thread-safe... */ #if JS_VERSION == 186 JS_TriggerOperationCallback (authority->priv->rt); @@ -993,6 +1013,9 @@ runaway_killer_setup (PolkitBackendJsAuthority *authority) g_assert (authority->priv->rkt_source == NULL); /* set-up timer for runaway scripts, will be executed in runaway_killer_thread */ + g_mutex_lock (&authority->priv->rkt_timeout_pending_mutex); + authority->priv->rkt_timeout_pending = FALSE; + g_mutex_unlock (&authority->priv->rkt_timeout_pending_mutex); authority->priv->rkt_source = g_timeout_source_new_seconds (15); g_source_set_callback (authority->priv->rkt_source, rkt_on_timeout, authority, NULL); g_source_attach (authority->priv->rkt_source, authority->priv->rkt_context);