Provide plugin module ordering guarantees

Rewrite the plugin internals so that modules have a well-defined
order--either the order of enable_only tags, or dynamic modules
followed by the built-in modules in order of registration.

ticket: 7665 (new)

3 files changed, 273 insertions, 200 deletions

diff --git a/doc/admin/conf_files/krb5_conf.rst b/doc/admin/conf_files/krb5_conf.rst
index 4f88c55662..0fd3f2c1d5 100644
--- a/doc/admin/conf_files/krb5_conf.rst
+++ b/doc/admin/conf_files/krb5_conf.rst
@@ -659,6 +659,12 @@ All subsections support the same tags:
     absolute path, it will be treated as relative to the
     **plugin_base_dir** value from :ref:`libdefaults`.
 
+For pluggable interfaces where module order matters, modules
+registered with a **module** tag normally come first, in the order
+they are registered, followed by built-in modules in the order they
+are documented below.  If **enable_only** tags are used, then the
+order of those tags overrides the normal module order.
+
 The following subsections are currently supported within the [plugins]
 section:
 
diff --git a/src/include/k5-int.h b/src/include/k5-int.h
index 73f404bdf7..63e0e8a32e 100644
--- a/src/include/k5-int.h
+++ b/src/include/k5-int.h
@@ -1057,21 +1057,11 @@ krb5_authdata_free_internal(krb5_context kcontext,
  * and krb5.conf man page.
  */
 
-/*
- * A linked list entry mapping a module name to a module initvt function.  The
- * entry may also include a dynamic object handle so that it can be released
- * when the context is destroyed.
- */
-struct plugin_mapping {
-    char *modname;
-    krb5_plugin_initvt_fn module;
-    struct plugin_file_handle *dyn_handle;
-    struct plugin_mapping *next;
-};
+struct plugin_mapping;
 
 /* Holds krb5_context information about each pluggable interface. */
 struct plugin_interface {
-    struct plugin_mapping *modules;
+    struct plugin_mapping **modules;
     krb5_boolean configured;
 };
 
diff --git a/src/lib/krb5/krb/plugin.c b/src/lib/krb5/krb/plugin.c
index 12945b4d65..059e00b994 100644
--- a/src/lib/krb5/krb/plugin.c
+++ b/src/lib/krb5/krb/plugin.c
@@ -26,6 +26,27 @@
 
 #include "k5-int.h"
 
+/*
+ * A plugin_mapping structure maps a module name to a built-in or dynamic
+ * module.  modname is always present; the other three fields can be in four
+ * different states:
+ *
+ * - If dyn_path and dyn_handle are null but module is set, the mapping is to a
+ *   built-in module.
+ * - If dyn_path is set but dyn_handle and module are null, the mapping is to a
+ *   dynamic module which hasn't been loaded yet.
+ * - If all three fields are set, the mapping is to a dynamic module which has
+ *   been loaded and is ready to use.
+ * - If all three fields are null, the mapping is to a dynamic module which
+ *   failed to load and should be ignored.
+ */
+struct plugin_mapping {
+    char *modname;
+    char *dyn_path;
+    struct plugin_file_handle *dyn_handle;
+    krb5_plugin_initvt_fn module;
+};
+
 const char *interface_names[] = {
     "pwqual",
     "kadm5_hook",
@@ -44,69 +65,96 @@ get_interface(krb5_context context, int id)
     return &context->plugins[id];
 }
 
-/* Release the memory associated with the linked list entry map. */
+/* Release the memory associated with the mapping list entry map. */
 static void
 free_plugin_mapping(struct plugin_mapping *map)
 {
     if (map == NULL)
         return;
     free(map->modname);
+    free(map->dyn_path);
     if (map->dyn_handle != NULL)
         krb5int_close_plugin(map->dyn_handle);
     free(map);
 }
 
+static void
+free_mapping_list(struct plugin_mapping **list)
+{
+    struct plugin_mapping **mp;
+
+    for (mp = list; mp != NULL && *mp != NULL; mp++)
+        free_plugin_mapping(*mp);
+    free(list);
+}
+
+/* Construct a plugin mapping object.  path may be NULL (for a built-in
+ * module), or may be relative to the plugin base directory. */
+static krb5_error_code
+make_plugin_mapping(krb5_context context, const char *name, size_t namelen,
+                    const char *path, krb5_plugin_initvt_fn module,
+                    struct plugin_mapping **map_out)
+{
+    krb5_error_code ret;
+    struct plugin_mapping *map = NULL;
+
+    /* Create the mapping entry. */
+    map = k5alloc(sizeof(*map), &ret);
+    if (map == NULL)
+        return ret;
+
+    map->modname = k5memdup0(name, namelen, &ret);
+    if (map->modname == NULL)
+        goto oom;
+    if (path != NULL) {
+        if (k5_path_join(context->plugin_base_dir, path, &map->dyn_path))
+            goto oom;
+    }
+    map->module = module;
+    *map_out = map;
+    return 0;
+
+oom:
+    free_plugin_mapping(map);
+    return ENOMEM;
+}
+
 /*
- * Register a mapping from modname to module.  On success, dyn_handle is
- * remembered in the mapping and will be released when the mapping is
- * overwritten or the context is destroyed.
+ * Register a mapping from modname to either dyn_path (for an auto-registered
+ * dynamic module) or to module (for a builtin module).  dyn_path may be
+ * relative to the plugin base directory.
  */
 static krb5_error_code
 register_module(krb5_context context, struct plugin_interface *interface,
-                const char *modname, krb5_plugin_initvt_fn module,
-                struct plugin_file_handle *dyn_handle)
+                const char *modname, const char *dyn_path,
+                krb5_plugin_initvt_fn module)
 {
-    struct plugin_mapping *map, **pmap;
-
-    /* If a mapping already exists for modname, remove it. */
-    for (pmap = &interface->modules; *pmap != NULL; pmap = &(*pmap)->next) {
-        map = *pmap;
-        if (strcmp(map->modname, modname) == 0) {
-            *pmap = map->next;
-            free_plugin_mapping(map);
-            break;
-        }
-    }
+    struct plugin_mapping **list;
+    size_t count;
 
-    /* Create a new mapping structure. */
-    map = malloc(sizeof(*map));
-    if (map == NULL)
-        return ENOMEM;
-    map->modname = strdup(modname);
-    if (map->modname == NULL) {
-        free(map);
+    /* Allocate list space for another element and a terminator. */
+    list = interface->modules;
+    for (count = 0; list != NULL && list[count] != NULL; count++);
+    list = realloc(interface->modules, (count + 2) * sizeof(*list));
+    if (list == NULL)
         return ENOMEM;
-    }
-    map->module = module;
-    map->dyn_handle = dyn_handle;
+    list[count] = list[count + 1] = NULL;
+    interface->modules = list;
 
-    /* Chain it into the list. */
-    map->next = interface->modules;
-    interface->modules = map;
-    return 0;
+    /* Create a new mapping structure and add it to the list. */
+    return make_plugin_mapping(context, modname, strlen(modname), dyn_path,
+                               module, &list[count]);
 }
 
-/* Parse a profile module string of the form "modname:modpath" into its
- * component parts. */
+/* Parse a profile module string of the form "modname:modpath" into a mapping
+ * entry. */
 static krb5_error_code
 parse_modstr(krb5_context context, const char *modstr,
-             char **modname, char **modpath)
+             struct plugin_mapping **map_out)
 {
     const char *sep;
-    char *name = NULL, *path = NULL;
 
-    *modname = NULL;
-    *modpath = NULL;
+    *map_out = NULL;
 
     sep = strchr(modstr, ':');
     if (sep == NULL) {
@@ -115,23 +163,8 @@ parse_modstr(krb5_context context, const char *modstr,
         return KRB5_PLUGIN_BAD_MODULE_SPEC;
     }
 
-    /* Copy the module name. */
-    name = malloc(sep - modstr + 1);
-    if (name == NULL)
-        return ENOMEM;
-    memcpy(name, modstr, sep - modstr);
-    name[sep - modstr] = '\0';
-
-    /* Copy the module path. */
-    path = strdup(sep + 1);
-    if (path == NULL) {
-        free(name);
-        return ENOMEM;
-    }
-
-    *modname = name;
-    *modpath = path;
-    return 0;
+    return make_plugin_mapping(context, modstr, sep - modstr, sep + 1, NULL,
+                               map_out);
 }
 
 /* Return true if value is found in list. */
@@ -145,108 +178,126 @@ find_in_list(char **list, const char *value)
     return FALSE;
 }
 
-/* Return true if module is not filtered out by enable or disable lists. */
-static krb5_boolean
-module_enabled(const char *modname, char **enable, char **disable)
+/* Get the list of values for the profile variable varname in the section for
+ * interface id, or NULL if no values are set. */
+static krb5_error_code
+get_profile_var(krb5_context context, int id, const char *varname, char ***out)
 {
-    return ((enable == NULL || find_in_list(enable, modname)) &&
-            (disable == NULL || !find_in_list(disable, modname)));
-}
+    krb5_error_code ret;
+    const char *path[4];
 
-/* Remove any registered modules whose names are filtered out. */
-static void
-filter_builtins(krb5_context context, struct plugin_interface *interface,
-                char **enable, char **disable)
-{
-    struct plugin_mapping *map, **pmap;
-
-    pmap = &interface->modules;
-    while (*pmap != NULL) {
-        map = *pmap;
-        if (!module_enabled(map->modname, enable, disable)) {
-            *pmap = map->next;
-            free_plugin_mapping(map);
-        } else
-            pmap = &map->next;
-    }
+    *out = NULL;
+    path[0] = KRB5_CONF_PLUGINS;
+    path[1] = interface_names[id];
+    path[2] = varname;
+    path[3] = NULL;
+    ret = profile_get_values(context->profile, path, out);
+    return (ret == PROF_NO_RELATION) ? 0 : ret;
 }
 
+/* Expand *list_inout to contain the mappings from modstrs, followed by the
+ * existing built-in module mappings. */
 static krb5_error_code
-register_dyn_module(krb5_context context, struct plugin_interface *interface,
-                    const char *iname, const char *modname, const char *path)
+make_full_list(krb5_context context, char **modstrs,
+               struct plugin_mapping ***list_inout)
 {
-    krb5_error_code ret;
-    char *symname = NULL;
-    struct plugin_file_handle *handle = NULL;
-    void (*initvt_fn)();
+    krb5_error_code ret = 0;
+    size_t count, pos, i, j;
+    struct plugin_mapping **list, **mp;
+    char **mod;
+
+    /* Allocate space for all of the modules plus a null terminator. */
+    for (count = 0; modstrs[count] != NULL; count++);
+    for (mp = *list_inout; mp != NULL && *mp != NULL; mp++, count++);
+    list = calloc(count + 1, sizeof(*list));
+    if (list == NULL)
+        return ENOMEM;
 
-    /* Construct the initvt symbol name for this interface and module. */
-    if (asprintf(&symname, "%s_%s_initvt", iname, modname) < 0) {
-        symname = NULL;
-        ret = ENOMEM;
-        goto cleanup;
+    /* Parse each profile module entry and store it in the list. */
+    for (mod = modstrs, pos = 0; *mod != NULL; mod++, pos++) {
+        ret = parse_modstr(context, *mod, &list[pos]);
+        if (ret != 0) {
+            free_mapping_list(list);
+            return ret;
+        }
     }
 
-    /* Open the plugin and resolve the initvt symbol. */
-    ret = krb5int_open_plugin(path, &handle, &context->err);
-    if (ret != 0)
-        goto cleanup;
-    ret = krb5int_get_plugin_func(handle, symname, &initvt_fn, &context->err);
-    if (ret != 0)
-        goto cleanup;
-
-    /* Create a mapping for the module. */
-    ret = register_module(context, interface, modname,
-                          (krb5_plugin_initvt_fn)initvt_fn, handle);
-    if (ret != 0)
-        goto cleanup;
-    handle = NULL;              /* Now owned by the module mapping. */
+    /* Cannibalize the old list of built-in modules. */
+    for (mp = *list_inout; mp != NULL && *mp != NULL; mp++, pos++)
+        list[pos] = *mp;
+    assert(pos == count);
+
+    /* Filter out duplicates, preferring earlier entries to later ones. */
+    for (i = 0, pos = 0; i < count; i++) {
+        for (j = 0; j < pos; j++) {
+            if (strcmp(list[i]->modname, list[j]->modname) == 0) {
+                free_plugin_mapping(list[i]);
+                break;
+            }
+        }
+        if (j == pos)
+            list[pos++] = list[i];
+    }
+    list[pos] = NULL;
 
-cleanup:
-    free(symname);
-    if (handle != NULL)
-        krb5int_close_plugin(handle);
-    return ret;
+    free(*list_inout);
+    *list_inout = list;
+    return 0;
 }
 
-/* Register the plugin module given by the profile string mod, if enabled
- * according to the values of enable and disable. */
-static krb5_error_code
-register_dyn_mapping(krb5_context context, struct plugin_interface *interface,
-                     const char *iname, const char *modstr, char **enable,
-                     char **disable)
+/* Remove any entries from list which match values in disabled. */
+static void
+remove_disabled_modules(struct plugin_mapping **list, char **disable)
 {
-    krb5_error_code ret;
-    char *modname = NULL, *modpath = NULL, *fullpath = NULL;
+    struct plugin_mapping **in, **out;
+
+    out = list;
+    for (in = list; *in != NULL; in++) {
+        if (find_in_list(disable, (*in)->modname))
+            free_plugin_mapping(*in);
+        else
+            *out++ = *in;
+    }
+    *out = NULL;
+}
 
-    /* Parse out the module name and path, and make sure it is enabled. */
-    ret = parse_modstr(context, modstr, &modname, &modpath);
-    if (ret != 0)
-        goto cleanup;
-    /* Treat non-absolute modpaths as relative to the plugin base directory. */
-    ret = k5_path_join(context->plugin_base_dir, modpath, &fullpath);
-    if (ret != 0)
-        goto cleanup;
-    if (!module_enabled(modname, enable, disable))
-        goto cleanup;
-    ret = register_dyn_module(context, interface, iname, modname, fullpath);
+/* Modify list to include only the entries matching strings in enable, in
+ * the order they are listed there. */
+static void
+filter_enabled_modules(struct plugin_mapping **list, char **enable)
+{
+    size_t count, i, pos = 0;
+    struct plugin_mapping *tmp;
+
+    /* Count the number of existing entries. */
+    for (count = 0; list[count] != NULL; count++);
+
+    /* For each string in enable, look for a matching module. */
+    for (; *enable != NULL; enable++) {
+        for (i = pos; i < count; i++) {
+            if (strcmp(list[i]->modname, *enable) == 0) {
+                /* Swap the matching module into the next result position. */
+                tmp = list[pos];
+                list[pos++] = list[i];
+                list[i] = tmp;
+                break;
+            }
+        }
+    }
 
-cleanup:
-    free(modname);
-    free(modpath);
-    free(fullpath);
-    return ret;
+    /* Free all mappings which didn't match and terminate the list. */
+    for (i = pos; i < count; i++)
+        free_plugin_mapping(list[i]);
+    list[pos] = NULL;
 }
 
-/* Ensure that a plugin interface is configured.  id is assumed to be valid. */
+/* Ensure that a plugin interface is configured.  id must be valid. */
 static krb5_error_code
 configure_interface(krb5_context context, int id)
 {
     krb5_error_code ret;
     struct plugin_interface *interface = &context->plugins[id];
-    const char *iname = interface_names[id];
-    char **modules = NULL, **enable = NULL, **disable = NULL, **mod;
-    static const char *path[4];
+    char **modstrs = NULL, **enable = NULL, **disable = NULL;
 
     if (interface->configured)
         return 0;
@@ -255,59 +306,93 @@ configure_interface(krb5_context context, int id)
     assert(sizeof(interface_names) / sizeof(*interface_names) ==
            PLUGIN_NUM_INTERFACES);
 
-    /* Read the configuration variables for this interface. */
-    path[0] = KRB5_CONF_PLUGINS;
-    path[1] = iname;
-    path[2] = KRB5_CONF_MODULE;
-    path[3] = NULL;
-    ret = profile_get_values(context->profile, path, &modules);
-    if (ret != 0 && ret != PROF_NO_RELATION)
+    /* Get profile variables for this interface. */
+    ret = get_profile_var(context, id, KRB5_CONF_MODULE, &modstrs);
+    if (ret)
         goto cleanup;
-    path[2] = KRB5_CONF_ENABLE_ONLY;
-    ret = profile_get_values(context->profile, path, &enable);
-    if (ret != 0 && ret != PROF_NO_RELATION)
+    ret = get_profile_var(context, id, KRB5_CONF_DISABLE, &disable);
+    if (ret)
         goto cleanup;
-    path[2] = KRB5_CONF_DISABLE;
-    ret = profile_get_values(context->profile, path, &disable);
-    if (ret != 0 && ret != PROF_NO_RELATION)
+    ret = get_profile_var(context, id, KRB5_CONF_ENABLE_ONLY, &enable);
+    if (ret)
         goto cleanup;
 
-    /* Remove built-in modules which are filtered out by configuration. */
-    filter_builtins(context, interface, enable, disable);
-
-    /* Create mappings for dynamic modules which aren't filtered out. */
-    for (mod = modules; mod && *mod; mod++) {
-        ret = register_dyn_mapping(context, interface, iname, *mod,
-                                   enable, disable);
-        if (ret != 0)
-            return ret;
+    /* Create the full list of dynamic and built-in modules. */
+    if (modstrs != NULL) {
+        ret = make_full_list(context, modstrs, &interface->modules);
+        if (ret)
+            goto cleanup;
     }
 
-    ret = 0;
+    /* Remove disabled modules. */
+    if (disable != NULL)
+        remove_disabled_modules(interface->modules, disable);
+
+    /* Filter and re-order the list according to enable-modules. */
+    if (enable != NULL)
+        filter_enabled_modules(interface->modules, enable);
+
 cleanup:
-    profile_free_list(modules);
+    profile_free_list(modstrs);
     profile_free_list(enable);
     profile_free_list(disable);
     return ret;
 }
 
+/* If map is for a dynamic module which hasn't been loaded yet, attempt to load
+ * it.  Only try to load a module once. */
+static void
+load_if_needed(krb5_context context, struct plugin_mapping *map,
+               const char *iname)
+{
+    char *symname = NULL;
+    struct plugin_file_handle *handle;
+    void (*initvt_fn)();
+
+    if (map->module != NULL || map->dyn_path == NULL)
+        return;
+    if (asprintf(&symname, "%s_%s_initvt", iname, map->modname) < 0)
+        return;
+    if (krb5int_open_plugin(map->dyn_path, &handle, &context->err))
+        goto err;
+    if (krb5int_get_plugin_func(handle, symname, &initvt_fn, &context->err))
+        goto err;
+    free(symname);
+    map->dyn_handle = handle;
+    map->module = (krb5_plugin_initvt_fn)initvt_fn;
+    return;
+
+err:
+    /* Clean up, and also null out map->dyn_path so we don't try again. */
+    if (handle != NULL)
+        krb5int_close_plugin(handle);
+    free(symname);
+    free(map->dyn_path);
+    map->dyn_path = NULL;
+}
+
 krb5_error_code
 k5_plugin_load(krb5_context context, int interface_id, const char *modname,
                krb5_plugin_initvt_fn *module)
 {
     krb5_error_code ret;
     struct plugin_interface *interface = get_interface(context, interface_id);
-    struct plugin_mapping *map;
+    struct plugin_mapping **mp, *map;
 
     if (interface == NULL)
         return EINVAL;
     ret = configure_interface(context, interface_id);
     if (ret != 0)
         return ret;
-    for (map = interface->modules; map != NULL; map = map->next) {
+    for (mp = interface->modules; mp != NULL && *mp != NULL; mp++) {
+        map = *mp;
         if (strcmp(map->modname, modname) == 0) {
-            *module = map->module;
-            return 0;
+            load_if_needed(context, map, interface_names[interface_id]);
+            if (map->module != NULL) {
+                *module = map->module;
+                return 0;
+            }
+            break;
         }
     }
     krb5_set_error_message(context, KRB5_PLUGIN_NAME_NOTFOUND,
@@ -322,7 +407,7 @@ k5_plugin_load_all(krb5_context context, int interface_id,
 {
     krb5_error_code ret;
     struct plugin_interface *interface = get_interface(context, interface_id);
-    struct plugin_mapping *map;
+    struct plugin_mapping **mp, *map;
     krb5_plugin_initvt_fn *list;
     size_t count;
 
@@ -333,18 +418,20 @@ k5_plugin_load_all(krb5_context context, int interface_id,
         return ret;
 
     /* Count the modules and allocate a list to hold them. */
-    count = 0;
-    for (map = interface->modules; map != NULL; map = map->next)
-        count++;
-    list = malloc((count + 1) * sizeof(*list));
+    mp = interface->modules;
+    for (count = 0; mp != NULL && mp[count] != NULL; count++);
+    list = calloc(count + 1, sizeof(*list));
     if (list == NULL)
         return ENOMEM;
 
     /* Place each module's initvt function into list. */
     count = 0;
-    for (map = interface->modules; map != NULL; map = map->next)
-        list[count++] = map->module;
-    list[count] = NULL;
+    for (mp = interface->modules; mp != NULL && *mp != NULL; mp++) {
+        map = *mp;
+        load_if_needed(context, map, interface_names[interface_id]);
+        if (map->module != NULL)
+            list[count++] = map->module;
+    }
 
     *modules = list;
     return 0;
@@ -370,7 +457,7 @@ k5_plugin_register(krb5_context context, int interface_id, const char *modname,
     if (interface->configured)
         return EINVAL;
 
-    return register_module(context, interface, modname, module, NULL);
+    return register_module(context, interface, modname, NULL, module);
 }
 
 krb5_error_code
@@ -384,12 +471,10 @@ k5_plugin_register_dyn(krb5_context context, int interface_id,
     /* Disallow registering plugins after load. */
     if (interface == NULL || interface->configured)
         return EINVAL;
-    if (asprintf(&path, "%s/%s/%s%s", context->plugin_base_dir, modsubdir,
-                 modname, PLUGIN_EXT) < 0)
-        return ENOMEM;
 
-    ret = register_dyn_module(context, interface,
-                              interface_names[interface_id], modname, path);
+    if (asprintf(&path, "%s/%s%s", modsubdir, modname, PLUGIN_EXT) < 0)
+        return ENOMEM;
+    ret = register_module(context, interface, modname, path, NULL);
     free(path);
     return ret;
 }
@@ -398,16 +483,8 @@ void
 k5_plugin_free_context(krb5_context context)
 {
     int i;
-    struct plugin_interface *interface;
-    struct plugin_mapping *map, *next;
-
-    for (i = 0; i < PLUGIN_NUM_INTERFACES; i++) {
-        interface = &context->plugins[i];
-        for (map = interface->modules; map != NULL; map = next) {
-            next = map->next;
-            free_plugin_mapping(map);
-        }
-        interface->modules = NULL;
-        interface->configured = FALSE;
-    }
+
+    for (i = 0; i < PLUGIN_NUM_INTERFACES; i++)
+        free_mapping_list(context->plugins[i].modules);
+    memset(context->plugins, 0, sizeof(context->plugins));
 }