Attachment 64283 Details for Bug 52158 – Use '.module' extension on module configs

[patch] Use '.module' extension on module configs

Use-module-extension-on-module-configs.patch (text/plain), 10.27 KB, created by Stef Walter on 2012-07-16 16:01:37 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Stef Walter

Created: 2012-07-16 16:01:37 UTC

Size: 10.27 KB

patch

obsolete

>From f29dcd2d7c2c07e6370445710ca2aa2bb7e662db Mon Sep 17 00:00:00 2001
>From: Stef Walter <stefw@gnome.org>
>Date: Mon, 16 Jul 2012 17:56:24 +0200
>Subject: [PATCH] Use '.module' extension on module configs
>
> * And want alphanumeric/_.- filenames
> * Currently this is just a warning, soon it will be enforced
> * The name of a module does not include the extension
>
>Andreas Metzler and Ubuntu both worked on this patch, and I've made
>some more changes.
>
>See https://bugs.launchpad.net/ubuntu/+source/p11-kit/+bug/911436
>
>https://bugs.freedesktop.org/show_bug.cgi?id=52158
>---
> doc/p11-kit-config.xml                             |    7 ++-
> p11-kit/conf.c                                     |   56 ++++++++++++++++++--
> tests/conf-test.c                                  |   23 +++++---
> tests/files/system-modules/{one => one.module}     |    0
> tests/files/system-modules/two                     |    5 --
> .../{two-duplicate => two-duplicate.module}        |    0
> tests/files/system-modules/two.badname             |    7 +++
> tests/files/user-modules/{one => one.module}       |    0
> tests/files/user-modules/{three => three.module}   |    0
> tests/test-modules.c                               |    4 +-
> 10 files changed, 82 insertions(+), 20 deletions(-)
> rename tests/files/system-modules/{one => one.module} (100%)
> delete mode 100644 tests/files/system-modules/two
> rename tests/files/system-modules/{two-duplicate => two-duplicate.module} (100%)
> create mode 100644 tests/files/system-modules/two.badname
> rename tests/files/user-modules/{one => one.module} (100%)
> rename tests/files/user-modules/{three => three.module} (100%)
>
>diff --git a/doc/p11-kit-config.xml b/doc/p11-kit-config.xml
>index 11fb41f..a10cb7e 100644
>--- a/doc/p11-kit-config.xml
>+++ b/doc/p11-kit-config.xml
>@@ -113,8 +113,11 @@ critical: yes
> 		<title>Module Configuration</title>
> 
> 		<para>Each configured PKCS#11 module has its own config file. These files
>-		can be <link linkend="config-locations">placed in various locations</link>.
>-		Most importantly each config file specifies the path of the PKCS#11 module to
>+		can be <link linkend="config-locations">placed in various locations</link>.</para>
>+		<para>The filename of the configuration file may consist of upper and lowercase letters
>+		underscore, comma, dash and dots. The first characters needs to be an alphanumeric,
>+		the filename should end with a <literal>.module</literal> extension.</para>
>+		<para>Most importantly each config file specifies the path of the PKCS#11 module to
> 		load. A module config file has the following fields:</para>
> 
> 		<variablelist>
>diff --git a/p11-kit/conf.c b/p11-kit/conf.c
>index fdb591d..132c679 100644
>--- a/p11-kit/conf.c
>+++ b/p11-kit/conf.c
>@@ -458,6 +458,46 @@ finished:
> 	return result;
> }
> 
>+static char *
>+calc_name_from_filename (const char *fname)
>+{
>+	/* We eventually want to settle on .module */
>+	static const char *suffix = ".module";
>+	static size_t suffix_len = 7;
>+	const char *c = fname;
>+	size_t fname_len;
>+	size_t name_len;
>+	char *name;
>+
>+	assert (fname);
>+
>+	/* Make sure the filename starts with an alphanumeric */
>+	if (!isalnum(*c))
>+		return NULL;
>+	++c;
>+
>+	/* Only allow alnum, _, -, and . */
>+	while (*c) {
>+		if (!isalnum(*c) && *c != '_' && *c != '-' && *c != '.')
>+			return NULL;
>+		++c;
>+	}
>+
>+	/* Make sure we have one of the suffixes */
>+	fname_len = strlen (fname);
>+	if (suffix_len < fname_len) {
>+		name_len = (fname_len - suffix_len);
>+		if (strcmp (fname + name_len, suffix) != 0)
>+			return NULL;
>+	}
>+
>+	name = malloc (name_len + 1);
>+	return_val_if_fail (name != NULL, NULL);
>+	memcpy (name, fname, name_len);
>+	name[name_len] = 0;
>+	return name;
>+}
>+
> static int
> load_config_from_file (const char *configfile, const char *name, hashmap *configs)
> {
>@@ -468,20 +508,28 @@ load_config_from_file (const char *configfile, const char *name, hashmap *config
> 
> 	assert (configfile);
> 
>+	key = calc_name_from_filename (name);
>+	if (key == NULL) {
>+		_p11_message ("invalid config filename, will be ignored in the future: %s", configfile);
>+		key = strdup (name);
>+		return_val_if_fail (key != NULL, -1);
>+	}
>+
> 	config = _p11_conf_parse_file (configfile, 0);
>-	if (!config)
>+	if (!config) {
>+		free (key);
> 		return -1;
>+	}
> 
>-	prev = _p11_hash_get (configs, name);
>+	prev = _p11_hash_get (configs, key);
> 	if (prev == NULL) {
>-		key = strdup (name);
>-		return_val_if_fail (key != NULL, -1);
> 		if (!_p11_hash_set (configs, key, config))
> 			return_val_if_reached (-1);
> 		config = NULL;
> 	} else {
> 		if (_p11_conf_merge_defaults (prev, config) < 0)
> 			error = errno;
>+		free (key);
> 	}
> 
> 	/* If still set */
>diff --git a/tests/conf-test.c b/tests/conf-test.c
>index eb04c71..92f7205 100644
>--- a/tests/conf-test.c
>+++ b/tests/conf-test.c
>@@ -246,6 +246,15 @@ test_load_globals_user_sets_invalid (CuTest *tc)
> 	_p11_hash_free (config);
> }
> 
>+static int
>+assert_msg_contains (const char *msg,
>+                     const char *text)
>+{
>+	if (msg == NULL)
>+		return 0;
>+	return strstr (msg, text) ? 1 : 0;
>+}
>+
> static void
> test_load_modules_merge (CuTest *tc)
> {
>@@ -258,14 +267,14 @@ test_load_modules_merge (CuTest *tc)
> 	                                  SRCDIR "/files/system-modules",
> 	                                  SRCDIR "/files/user-modules");
> 	CuAssertPtrNotNull (tc, configs);
>-	CuAssertStrEquals (tc, NULL, p11_kit_message ());
>+	CuAssertTrue (tc, assert_msg_contains (p11_kit_message (), "invalid config filename"));
> 
> 	config = _p11_hash_get (configs, "one");
> 	CuAssertPtrNotNull (tc, config);
> 	CuAssertStrEquals (tc, "mock-one.so", _p11_hash_get (config, "module"));
> 	CuAssertStrEquals (tc, _p11_hash_get (config, "setting"), "user1");
> 
>-	config = _p11_hash_get (configs, "two");
>+	config = _p11_hash_get (configs, "two.badname");
> 	CuAssertPtrNotNull (tc, config);
> 	CuAssertStrEquals (tc, "mock-two.so", _p11_hash_get (config, "module"));
> 	CuAssertStrEquals (tc, _p11_hash_get (config, "setting"), "system2");
>@@ -290,14 +299,14 @@ test_load_modules_user_none (CuTest *tc)
> 	                                  SRCDIR "/files/system-modules",
> 	                                  SRCDIR "/files/user-modules");
> 	CuAssertPtrNotNull (tc, configs);
>-	CuAssertStrEquals (tc, NULL, p11_kit_message ());
>+	CuAssertTrue (tc, assert_msg_contains (p11_kit_message (), "invalid config filename"));
> 
> 	config = _p11_hash_get (configs, "one");
> 	CuAssertPtrNotNull (tc, config);
> 	CuAssertStrEquals (tc, "mock-one.so", _p11_hash_get (config, "module"));
> 	CuAssertStrEquals (tc, _p11_hash_get (config, "setting"), "system1");
> 
>-	config = _p11_hash_get (configs, "two");
>+	config = _p11_hash_get (configs, "two.badname");
> 	CuAssertPtrNotNull (tc, config);
> 	CuAssertStrEquals (tc, "mock-two.so", _p11_hash_get (config, "module"));
> 	CuAssertStrEquals (tc, _p11_hash_get (config, "setting"), "system2");
>@@ -327,7 +336,7 @@ test_load_modules_user_only (CuTest *tc)
> 	CuAssertStrEquals (tc, _p11_hash_get (config, "module"), NULL);
> 	CuAssertStrEquals (tc, _p11_hash_get (config, "setting"), "user1");
> 
>-	config = _p11_hash_get (configs, "two");
>+	config = _p11_hash_get (configs, "two.badname");
> 	CuAssertPtrEquals (tc, NULL, config);
> 
> 	config = _p11_hash_get (configs, "three");
>@@ -350,14 +359,14 @@ test_load_modules_no_user (CuTest *tc)
> 	                                  SRCDIR "/files/system-modules",
> 	                                  SRCDIR "/files/non-existant");
> 	CuAssertPtrNotNull (tc, configs);
>-	CuAssertStrEquals (tc, NULL, p11_kit_message ());
>+	CuAssertTrue (tc, assert_msg_contains (p11_kit_message (), "invalid config filename"));
> 
> 	config = _p11_hash_get (configs, "one");
> 	CuAssertPtrNotNull (tc, config);
> 	CuAssertStrEquals (tc, "mock-one.so", _p11_hash_get (config, "module"));
> 	CuAssertStrEquals (tc, _p11_hash_get (config, "setting"), "system1");
> 
>-	config = _p11_hash_get (configs, "two");
>+	config = _p11_hash_get (configs, "two.badname");
> 	CuAssertPtrNotNull (tc, config);
> 	CuAssertStrEquals (tc, "mock-two.so", _p11_hash_get (config, "module"));
> 	CuAssertStrEquals (tc, _p11_hash_get (config, "setting"), "system2");
>diff --git a/tests/files/system-modules/one b/tests/files/system-modules/one.module
>similarity index 100%
>rename from tests/files/system-modules/one
>rename to tests/files/system-modules/one.module
>diff --git a/tests/files/system-modules/two b/tests/files/system-modules/two
>deleted file mode 100644
>index f391757..0000000
>--- a/tests/files/system-modules/two
>+++ /dev/null
>@@ -1,5 +0,0 @@
>-
>-module: mock-two.so
>-setting: system2
>-
>-disable-in: test-disable, test-other
>\ No newline at end of file
>diff --git a/tests/files/system-modules/two-duplicate b/tests/files/system-modules/two-duplicate.module
>similarity index 100%
>rename from tests/files/system-modules/two-duplicate
>rename to tests/files/system-modules/two-duplicate.module
>diff --git a/tests/files/system-modules/two.badname b/tests/files/system-modules/two.badname
>new file mode 100644
>index 0000000..b6d0f7f
>--- /dev/null
>+++ b/tests/files/system-modules/two.badname
>@@ -0,0 +1,7 @@
>+# This module doesn't have a .module extension, but p11-kit doesn't yet
>+# enforce the naming, just warns, so it should still be loaded
>+
>+module: mock-two.so
>+setting: system2
>+
>+disable-in: test-disable, test-other
>\ No newline at end of file
>diff --git a/tests/files/user-modules/one b/tests/files/user-modules/one.module
>similarity index 100%
>rename from tests/files/user-modules/one
>rename to tests/files/user-modules/one.module
>diff --git a/tests/files/user-modules/three b/tests/files/user-modules/three.module
>similarity index 100%
>rename from tests/files/user-modules/three
>rename to tests/files/user-modules/three.module
>diff --git a/tests/test-modules.c b/tests/test-modules.c
>index 1debed0..f755298 100644
>--- a/tests/test-modules.c
>+++ b/tests/test-modules.c
>@@ -142,7 +142,7 @@ test_disable (CuTest *tc)
> 	 */
> 
> 	modules = initialize_and_get_modules (tc);
>-	CuAssertTrue (tc, lookup_module_with_name (tc, modules, "two") != NULL);
>+	CuAssertTrue (tc, lookup_module_with_name (tc, modules, "two.badname") != NULL);
> 	finalize_and_free_modules (tc, modules);
> 
> 	/*
>@@ -155,7 +155,7 @@ test_disable (CuTest *tc)
> 	p11_kit_set_progname ("test-disable");
> 
> 	modules = initialize_and_get_modules (tc);
>-	CuAssertTrue (tc, lookup_module_with_name (tc, modules, "two") == NULL);
>+	CuAssertTrue (tc, lookup_module_with_name (tc, modules, "two.badname") == NULL);
> 	finalize_and_free_modules (tc, modules);
> 
> 	p11_kit_set_progname (NULL);
>-- 
>1.7.10.4

Actions: View | Splinter Review

Attachments on bug 52158: 64283 | 64292

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.