From 9d27c57f7035a6bb977070abadacfa479a0237d9 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 10 Feb 2021 21:16:39 +0000 Subject: [PATCH 1/3] gkeyfilesettingsbackend: Fix basename handling when group is unset Fix an effective regression in commit 7781a9cbd2fd0aa84bee0f4eee88470640ff6706, which happens when `convert_path()` is called with a `key` which contains no slashes. In that case, the `key` is entirely the `basename`. Prior to commit 7781a9cb, the code worked through a fluke of `i == -1` cancelling out with the various additions in the `g_memdup()` call, and effectively resulting in `g_strdup (key)`. Spotted by Guido Berhoerster. Signed-off-by: Philip Withnall --- gio/gkeyfilesettingsbackend.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/gio/gkeyfilesettingsbackend.c b/gio/gkeyfilesettingsbackend.c index 793eed02a..d57a9d421 100644 --- a/gio/gkeyfilesettingsbackend.c +++ b/gio/gkeyfilesettingsbackend.c @@ -188,7 +188,12 @@ convert_path (GKeyfileSettingsBackend *kfsb, } if (basename) - *basename = g_memdup2 (last_slash + 1, key_len - (last_slash - key)); + { + if (last_slash != NULL) + *basename = g_memdup2 (last_slash + 1, key_len - (last_slash - key)); + else + *basename = g_strdup (key); + } return TRUE; } From a59247608feee2adcefafbbb774ed5f7d5275219 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 10 Feb 2021 21:19:30 +0000 Subject: [PATCH 2/3] gkeyfilesettingsbackend: Disallow empty key or group names These should never have been allowed; they will result in precondition failures from the `GKeyFile` later on in the code. A test will be added for this shortly. Signed-off-by: Philip Withnall --- gio/gkeyfilesettingsbackend.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/gio/gkeyfilesettingsbackend.c b/gio/gkeyfilesettingsbackend.c index d57a9d421..485a25a14 100644 --- a/gio/gkeyfilesettingsbackend.c +++ b/gio/gkeyfilesettingsbackend.c @@ -161,6 +161,13 @@ convert_path (GKeyfileSettingsBackend *kfsb, last_slash = strrchr (key, '/'); + /* Disallow empty group names or key names */ + if (key_len == 0 || + (last_slash != NULL && + (*(last_slash + 1) == '\0' || + last_slash == key))) + return FALSE; + if (kfsb->root_group) { /* if a root_group was specified, make sure the user hasn't given From 68ce7a28e1c44c9750b586ace04474769f1b3c2e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 10 Feb 2021 21:21:36 +0000 Subject: [PATCH 3/3] tests: Add tests for key name handling in the keyfile backend This tests the two recent commits. Signed-off-by: Philip Withnall --- gio/tests/gsettings.c | 170 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 169 insertions(+), 1 deletion(-) diff --git a/gio/tests/gsettings.c b/gio/tests/gsettings.c index 16eb228c4..9e570a1b2 100644 --- a/gio/tests/gsettings.c +++ b/gio/tests/gsettings.c @@ -1740,6 +1740,14 @@ key_changed_cb (GSettings *settings, const gchar *key, gpointer data) (*b) = TRUE; } +typedef struct +{ + const gchar *path; + const gchar *root_group; + const gchar *keyfile_group; + const gchar *root_path; +} KeyfileTestData; + /* * Test that using a keyfile works */ @@ -1834,7 +1842,11 @@ test_keyfile (Fixture *fixture, g_free (str); g_settings_set (settings, "farewell", "s", "cheerio"); - + + /* Check that empty keys/groups are not allowed. */ + g_assert_false (g_settings_is_writable (settings, "")); + g_assert_false (g_settings_is_writable (settings, "/")); + /* When executing as root, changing the mode of the keyfile will have * no effect on the writability of the settings. */ @@ -1866,6 +1878,149 @@ test_keyfile (Fixture *fixture, g_free (keyfile_path); } +/* + * Test that using a keyfile works with a schema with no path set. + */ +static void +test_keyfile_no_path (Fixture *fixture, + gconstpointer user_data) +{ + const KeyfileTestData *test_data = user_data; + GSettingsBackend *kf_backend; + GSettings *settings; + GKeyFile *keyfile; + gboolean writable; + gchar *key = NULL; + GError *error = NULL; + gchar *keyfile_path = NULL, *store_path = NULL; + + keyfile_path = g_build_filename (fixture->tmp_dir, "keyfile", NULL); + store_path = g_build_filename (keyfile_path, "gsettings.store", NULL); + kf_backend = g_keyfile_settings_backend_new (store_path, test_data->root_path, test_data->root_group); + settings = g_settings_new_with_backend_and_path ("org.gtk.test.no-path", kf_backend, test_data->path); + g_object_unref (kf_backend); + + g_settings_reset (settings, "test-boolean"); + g_assert_true (g_settings_get_boolean (settings, "test-boolean")); + + writable = g_settings_is_writable (settings, "test-boolean"); + g_assert_true (writable); + g_settings_set (settings, "test-boolean", "b", FALSE); + + g_assert_false (g_settings_get_boolean (settings, "test-boolean")); + + g_settings_delay (settings); + g_settings_set (settings, "test-boolean", "b", TRUE); + g_settings_apply (settings); + + keyfile = g_key_file_new (); + g_assert_true (g_key_file_load_from_file (keyfile, store_path, 0, NULL)); + + g_assert_true (g_key_file_get_boolean (keyfile, test_data->keyfile_group, "test-boolean", NULL)); + + g_key_file_free (keyfile); + + g_settings_reset (settings, "test-boolean"); + g_settings_apply (settings); + keyfile = g_key_file_new (); + g_assert_true (g_key_file_load_from_file (keyfile, store_path, 0, NULL)); + + g_assert_false (g_key_file_get_string (keyfile, test_data->keyfile_group, "test-boolean", &error)); + g_assert_error (error, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_KEY_NOT_FOUND); + g_clear_error (&error); + + /* Check that empty keys/groups are not allowed. */ + g_assert_false (g_settings_is_writable (settings, "")); + g_assert_false (g_settings_is_writable (settings, "/")); + + /* Keys which ghost the root group name are not allowed. This can only be + * tested when the path is `/` as otherwise it acts as a prefix and prevents + * any ghosting. */ + if (g_str_equal (test_data->path, "/")) + { + key = g_strdup_printf ("%s/%s", test_data->root_group, ""); + g_assert_false (g_settings_is_writable (settings, key)); + g_free (key); + + key = g_strdup_printf ("%s/%s", test_data->root_group, "/"); + g_assert_false (g_settings_is_writable (settings, key)); + g_free (key); + + key = g_strdup_printf ("%s/%s", test_data->root_group, "test-boolean"); + g_assert_false (g_settings_is_writable (settings, key)); + g_free (key); + } + + g_key_file_free (keyfile); + g_object_unref (settings); + + /* Clean up the temporary directory. */ + g_assert_no_errno (g_chmod (keyfile_path, 0777)); + g_assert_no_errno (g_remove (store_path)); + g_assert_no_errno (g_rmdir (keyfile_path)); + g_free (store_path); + g_free (keyfile_path); +} + +/* + * Test that a keyfile rejects writes to keys outside its root path. + */ +static void +test_keyfile_outside_root_path (Fixture *fixture, + gconstpointer user_data) +{ + GSettingsBackend *kf_backend; + GSettings *settings; + gchar *keyfile_path = NULL, *store_path = NULL; + + keyfile_path = g_build_filename (fixture->tmp_dir, "keyfile", NULL); + store_path = g_build_filename (keyfile_path, "gsettings.store", NULL); + kf_backend = g_keyfile_settings_backend_new (store_path, "/tests/basic-types/", "root"); + settings = g_settings_new_with_backend_and_path ("org.gtk.test.no-path", kf_backend, "/tests/"); + g_object_unref (kf_backend); + + g_assert_false (g_settings_is_writable (settings, "test-boolean")); + + g_object_unref (settings); + + /* Clean up the temporary directory. The keyfile probably doesn’t exist, so + * don’t error on failure. */ + g_remove (store_path); + g_assert_no_errno (g_rmdir (keyfile_path)); + g_free (store_path); + g_free (keyfile_path); +} + +/* + * Test that a keyfile rejects writes to keys in the root if no root group is set. + */ +static void +test_keyfile_no_root_group (Fixture *fixture, + gconstpointer user_data) +{ + GSettingsBackend *kf_backend; + GSettings *settings; + gchar *keyfile_path = NULL, *store_path = NULL; + + keyfile_path = g_build_filename (fixture->tmp_dir, "keyfile", NULL); + store_path = g_build_filename (keyfile_path, "gsettings.store", NULL); + kf_backend = g_keyfile_settings_backend_new (store_path, "/", NULL); + settings = g_settings_new_with_backend_and_path ("org.gtk.test.no-path", kf_backend, "/"); + g_object_unref (kf_backend); + + g_assert_false (g_settings_is_writable (settings, "test-boolean")); + g_assert_true (g_settings_is_writable (settings, "child/test-boolean")); + + g_object_unref (settings); + + /* Clean up the temporary directory. The keyfile probably doesn’t exist, so + * don’t error on failure. */ + g_remove (store_path); + g_assert_no_errno (g_rmdir (keyfile_path)); + g_free (store_path); + g_free (keyfile_path); +} + /* Test that getting child schemas works */ static void @@ -2844,6 +2999,14 @@ main (int argc, char *argv[]) gchar *override_text; gchar *enums; gint result; + const KeyfileTestData keyfile_test_data_explicit_path = { "/tests/", "root", "tests", "/" }; + const KeyfileTestData keyfile_test_data_empty_path = { "/", "root", "root", "/" }; + const KeyfileTestData keyfile_test_data_long_path = { + "/tests/path/is/very/long/and/this/makes/some/comparisons/take/a/different/branch/", + "root", + "tests/path/is/very/long/and/this/makes/some/comparisons/take/a/different/branch", + "/" + }; /* Meson build sets this */ #ifdef TEST_LOCALE_PATH @@ -2967,6 +3130,11 @@ main (int argc, char *argv[]) } g_test_add ("/gsettings/keyfile", Fixture, NULL, setup, test_keyfile, teardown); + g_test_add ("/gsettings/keyfile/explicit-path", Fixture, &keyfile_test_data_explicit_path, setup, test_keyfile_no_path, teardown); + g_test_add ("/gsettings/keyfile/empty-path", Fixture, &keyfile_test_data_empty_path, setup, test_keyfile_no_path, teardown); + g_test_add ("/gsettings/keyfile/long-path", Fixture, &keyfile_test_data_long_path, setup, test_keyfile_no_path, teardown); + g_test_add ("/gsettings/keyfile/outside-root-path", Fixture, NULL, setup, test_keyfile_outside_root_path, teardown); + g_test_add ("/gsettings/keyfile/no-root-group", Fixture, NULL, setup, test_keyfile_no_root_group, teardown); g_test_add_func ("/gsettings/child-schema", test_child_schema); g_test_add_func ("/gsettings/strinfo", test_strinfo); g_test_add_func ("/gsettings/enums", test_enums);