From: Jennifer Richards <jennifer@painless-security.com>
Date: Fri, 8 Sep 2017 23:59:27 +0000 (-0400)
Subject: Rearrange config file loading to allow splitting across files
X-Git-Tag: v3.0.2~8
X-Git-Url: http://www.project-moonshot.org/gitweb/?p=trust_router.git;a=commitdiff_plain;h=7a6dc59919519fc7a7927301da0c079298258c92

Rearrange config file loading to allow splitting across files

Prior to this commit, configuration files were loaded one-by-one,
parsing all the sections of each file before moving to the next. This
caused problems unless the files were arranged so that realms were
defined before they were referred to by communities when the files
were read in lexical order. This commit rearranges this so that all
files are first parsed into internal JSON structures, then the first
section of all these structures is parsed, the second section of all
structures is parsed, etc. This eliminates the dependency on file
order that caused the bug.

Also fixed a memory leak: the JSON structures were not being properly
freed after being parsed. These should now be freed.
---

diff --git a/common/tr_config.c b/common/tr_config.c
index d48902b..b0d8b4c 100644
--- a/common/tr_config.c
+++ b/common/tr_config.c
@@ -2019,13 +2019,6 @@ TR_CFG_RC tr_cfg_validate(TR_CFG *trc)
   return rc;
 }
 
-/* Join two paths and return a pointer to the result. This should be freed
- * via talloc_free. Returns NULL on failure. */
-static char *join_paths(TALLOC_CTX *mem_ctx, const char *p1, const char *p2)
-{
-  return talloc_asprintf(mem_ctx, "%s/%s", p1, p2); /* returns NULL on a failure */
-}
-
 static void tr_cfg_log_json_error(const char *label, json_error_t *rc)
 {
   tr_debug("%s: JSON parse error on line %d: %s",
@@ -2034,7 +2027,14 @@ static void tr_cfg_log_json_error(const char *label, json_error_t *rc)
 	   rc->text);
 }
 
-TR_CFG_RC tr_cfg_parse_one_config_file(TR_CFG *cfg, const char *file_with_path)
+/**
+ * Parse a config file and return its JSON structure. Also emits a serial number to the log
+ * if one is present.
+ *
+ * @param file_with_path The file (with path!) to parse
+ * @return Pointer to the result of parsing, or null on error
+ */
+static json_t *tr_cfg_parse_one_config_file(const char *file_with_path)
 {
   json_t *jcfg=NULL;
   json_t *jser=NULL;
@@ -2045,10 +2045,10 @@ TR_CFG_RC tr_cfg_parse_one_config_file(TR_CFG *cfg, const char *file_with_path)
     tr_debug("tr_cfg_parse_one_config_file: Error parsing config file %s.", 
              file_with_path);
     tr_cfg_log_json_error("tr_cfg_parse_one_config_file", &rc);
-    return TR_CFG_NOPARSE;
+    return NULL;
   }
 
-  // Look for serial number and log it if it exists
+  // Look for serial number and log it if it exists (borrowed reference, so no need to free it later)
   if (NULL!=(jser=json_object_get(jcfg, "serial_number"))) {
     if (json_is_number(jser)) {
       tr_notice("tr_parse_one_config_file: Attempting to load revision %" JSON_INTEGER_FORMAT " of '%s'.",
@@ -2057,29 +2057,108 @@ TR_CFG_RC tr_cfg_parse_one_config_file(TR_CFG *cfg, const char *file_with_path)
     }
   }
 
-  if ((TR_CFG_SUCCESS != tr_cfg_parse_internal(cfg, jcfg)) ||
-      (TR_CFG_SUCCESS != tr_cfg_parse_local_orgs(cfg, jcfg)) ||
-      (TR_CFG_SUCCESS != tr_cfg_parse_peer_orgs(cfg, jcfg)) ||
-      (TR_CFG_SUCCESS != tr_cfg_parse_default_servers(cfg, jcfg)) ||
-      (TR_CFG_SUCCESS != tr_cfg_parse_comms(cfg, jcfg)))
-    return TR_CFG_ERROR;
+  return jcfg;
+}
 
-  return TR_CFG_SUCCESS;
+/**
+ * Helper to free an array returned by tr_cfg_parse_config_files
+ * @param n_jcfgs
+ * @param jcfgs
+ */
+static void tr_cfg_parse_free_jcfgs(unsigned int n_jcfgs, json_t **jcfgs)
+{
+  int ii=0;
+  for (ii=0; ii<n_jcfgs; ii++)
+    json_decref(jcfgs[ii]);
+  talloc_free(jcfgs);
 }
 
-/* Reads configuration files in config_dir ("" or "./" will use the current directory). */
-TR_CFG_RC tr_parse_config(TR_CFG_MGR *cfg_mgr, const char *config_dir, int n, struct dirent **cfg_files)
+/**
+ * Parse a list of configuration files. Returns an array of JSON objects, free this with
+ * tr_cfg_parse_free_jcfgs(), a helper function
+ *
+ * @param config_dir
+ * @param n_files
+ * @param cfg_files
+ * @return
+ */
+static json_t **tr_cfg_parse_config_files(TALLOC_CTX *mem_ctx, unsigned int n_files, char **files_with_paths)
 {
   TALLOC_CTX *tmp_ctx=talloc_new(NULL);
-  char *file_with_path;
-  int ii;
+  unsigned int ii=0;
+  json_t **jcfgs=NULL;
+
+  /* first allocate the jcfgs */
+  jcfgs=talloc_array(NULL, json_t *, n_files);
+  if (jcfgs==NULL) {
+    tr_crit("tr_parse_config_files: cannot allocate JSON structure array");
+    goto cleanup;
+  }
+  for (ii=0; ii<n_files; ii++) {
+    jcfgs[ii]=tr_cfg_parse_one_config_file(files_with_paths[ii]);
+    if (jcfgs[ii]==NULL) {
+      tr_err("tr_parse_config: Error parsing JSON in %s", files_with_paths[ii]);
+      tr_cfg_parse_free_jcfgs(ii, jcfgs); /* frees the JSON objects and the jcfgs array */
+      jcfgs=NULL;
+      goto cleanup;
+    }
+  }
+cleanup:
+  if (jcfgs)
+    talloc_steal(mem_ctx, jcfgs); /* give this to the caller's context if we succeeded */
+  talloc_free(tmp_ctx);
+  return jcfgs;
+}
+
+/* define a type for config parse functions */
+typedef TR_CFG_RC (TR_CFG_PARSE_FN)(TR_CFG *, json_t *);
+/**
+ * Helper function to parse a collection of JSON structures using a generic parse function.
+ *
+ * @param cfg Config structure to receive results
+ * @param jcfgs Pointer to an array of decoded JSON structures
+ * @param n_jcfg Number of JSON structures in the array
+ * @param parse_fn Function to apply
+ * @return TR_CFG_SUCCESS on success, _FAIL or an error code on failure
+ */
+static TR_CFG_RC tr_cfg_parse_helper(TR_CFG *cfg, json_t **jcfgs, size_t n_jcfg, TR_CFG_PARSE_FN parse_fn)
+{
+  size_t ii=0;
+  json_t *this_jcfg=NULL;
+  TR_CFG_RC ret=TR_CFG_ERROR;
+
+  /* TODO validate arguments */
+
+  for (ii=0; ii<n_jcfg; ii++) {
+    this_jcfg=jcfgs[ii];
+    ret=parse_fn(cfg, this_jcfg);
+    if (ret!=TR_CFG_SUCCESS)
+      break;
+  }
+  return ret;
+}
+
+
+/**
+ *  Reads configuration files in config_dir ("" or "./" will use the current directory).
+ *
+ * @param cfg_mgr Configuration manager
+ * @param n_files Number of entries in cfg_files
+ * @param files_with_paths Array of filenames with path to load
+ * @return TR_CFG_SUCCESS on success, TR_CFG_ERROR or a more specific error on failure
+ */
+TR_CFG_RC tr_parse_config(TR_CFG_MGR *cfg_mgr, unsigned int n_files, char **files_with_paths)
+{
+  TALLOC_CTX *tmp_ctx=talloc_new(NULL);
+  json_t **jcfgs=NULL;
   TR_CFG_RC cfg_rc=TR_CFG_ERROR;
 
-  if ((!cfg_mgr) || (!cfg_files) || (n<=0)) {
+  if ((!cfg_mgr) || (!files_with_paths)) {
     cfg_rc=TR_CFG_BAD_PARAMS;
     goto cleanup;
   }
 
+  /* get a fresh config to fill in, freeing old one if needed */
   if (cfg_mgr->new != NULL)
     tr_cfg_free(cfg_mgr->new);
   cfg_mgr->new=tr_cfg_new(tmp_ctx); /* belongs to the temporary context for now */
@@ -2088,24 +2167,22 @@ TR_CFG_RC tr_parse_config(TR_CFG_MGR *cfg_mgr, const char *config_dir, int n, st
     goto cleanup;
   }
 
+  /* first parse the json */
+  jcfgs=tr_cfg_parse_config_files(tmp_ctx, n_files, files_with_paths);
+  if (jcfgs==NULL) {
+    cfg_rc=TR_CFG_NOPARSE;
+    goto cleanup;
+  }
+
   cfg_mgr->new->peers=trp_ptable_new(cfg_mgr); /* not sure why this isn't in cfg_mgr->new's context */
 
-  /* Parse configuration information from each config file */
-  for (ii=0; ii<n; ii++) {
-    file_with_path=join_paths(tmp_ctx, config_dir, cfg_files[ii]->d_name); /* must free result with talloc_free */
-    if(file_with_path == NULL) {
-      tr_crit("tr_parse_config: error joining path.");
-      cfg_rc=TR_CFG_NOMEM;
-      goto cleanup;
-    }
-    tr_debug("tr_parse_config: Parsing %s.", cfg_files[ii]->d_name); /* print the filename without the path */
-    cfg_rc=tr_cfg_parse_one_config_file(cfg_mgr->new, file_with_path);
-    if (cfg_rc!=TR_CFG_SUCCESS) {
-      tr_crit("tr_parse_config: Error parsing %s", file_with_path);
-      goto cleanup;
-    }
-    talloc_free(file_with_path); /* done with filename */
-  }
+  /* now run through the parsers on the JSON */
+  if ((TR_CFG_SUCCESS != (cfg_rc=tr_cfg_parse_helper(cfg_mgr->new, jcfgs, n_files, tr_cfg_parse_internal))) ||
+      (TR_CFG_SUCCESS != (cfg_rc=tr_cfg_parse_helper(cfg_mgr->new, jcfgs, n_files, tr_cfg_parse_local_orgs))) ||
+      (TR_CFG_SUCCESS != (cfg_rc=tr_cfg_parse_helper(cfg_mgr->new, jcfgs, n_files, tr_cfg_parse_peer_orgs))) ||
+      (TR_CFG_SUCCESS != (cfg_rc=tr_cfg_parse_helper(cfg_mgr->new, jcfgs, n_files, tr_cfg_parse_default_servers))) ||
+      (TR_CFG_SUCCESS != (cfg_rc=tr_cfg_parse_helper(cfg_mgr->new, jcfgs, n_files, tr_cfg_parse_comms))))
+    goto cleanup; /* cfg_rc was set above */
 
   /* make sure we got a complete, consistent configuration */
   if (TR_CFG_SUCCESS != tr_cfg_validate(cfg_mgr->new)) {
@@ -2119,6 +2196,8 @@ TR_CFG_RC tr_parse_config(TR_CFG_MGR *cfg_mgr, const char *config_dir, int n, st
   cfg_rc=TR_CFG_SUCCESS;
 
 cleanup:
+  if (jcfgs!=NULL)
+    tr_cfg_parse_free_jcfgs(n_files, jcfgs);
   talloc_free(tmp_ctx);
   return cfg_rc;
 }
@@ -2197,7 +2276,7 @@ static int is_cfg_file(const struct dirent *dent) {
  * allocated array of pointers to struct dirent entries, as returned
  * by scandir(). These can be freed with tr_free_config_file_list().
  */
-int tr_find_config_files (const char *config_dir, struct dirent ***cfg_files) {
+int tr_find_config_files(const char *config_dir, struct dirent ***cfg_files) {
   int n = 0;
   
   n = scandir(config_dir, cfg_files, is_cfg_file, alphasort);
diff --git a/include/tr_config.h b/include/tr_config.h
index 62c5fa1..89fd328 100644
--- a/include/tr_config.h
+++ b/include/tr_config.h
@@ -100,10 +100,9 @@ typedef struct tr_cfg_mgr {
   TR_CFG *new;
 } TR_CFG_MGR;
 
-int tr_find_config_files (const char *config_dir, struct dirent ***cfg_files);
+int tr_find_config_files(const char *config_dir, struct dirent ***cfg_files);
 void tr_free_config_file_list(int n, struct dirent ***cfg_files);
-TR_CFG_RC tr_parse_config (TR_CFG_MGR *cfg_mgr, const char *config_dir, int n, struct dirent **cfg_files);
-TR_CFG_RC tr_cfg_parse_one_config_file(TR_CFG *cfg, const char *file_with_path);
+TR_CFG_RC tr_parse_config(TR_CFG_MGR *cfg_mgr, unsigned int n_files, char **files_with_paths);
 TR_CFG_RC tr_apply_new_config (TR_CFG_MGR *cfg_mgr);
 TR_CFG_RC tr_cfg_validate (TR_CFG *trc);
 TR_CFG *tr_cfg_new(TALLOC_CTX *mem_ctx);
diff --git a/tr/tr_cfgwatch.c b/tr/tr_cfgwatch.c
index 6b125bc..b0b7e34 100644
--- a/tr/tr_cfgwatch.c
+++ b/tr/tr_cfgwatch.c
@@ -186,15 +186,63 @@ static int tr_cfgwatch_update_needed(TR_CFGWATCH *cfg_status)
   return update_needed;
 }
 
+/* Join two paths and return a pointer to the result. This should be freed
+ * via talloc_free. Returns NULL on failure. */
+static char *join_paths(TALLOC_CTX *mem_ctx, const char *p1, const char *p2)
+{
+  return talloc_asprintf(mem_ctx, "%s/%s", p1, p2); /* returns NULL on a failure */
+}
+
+/**
+ * Join a directory name with the filenames from an array of struct dirent.
+ * Outputs an array of pointers to strings that must be freed via talloc_free on
+ * the array. The strings in the array are in the context of the array, so will
+ * be freed automatically.
+ *
+ * @param ctx talloc context to contain the result on success
+ * @param dir
+ * @param n_files
+ * @param files
+ * @return Null on failure, or an array of pointers to strings
+ */
+static char **dirent_to_full_path(TALLOC_CTX *mem_ctx, const char *dir, unsigned int n_files, struct dirent **files)
+{
+  TALLOC_CTX *tmp_ctx=talloc_new(NULL);
+  unsigned int ii=0;
+  char **files_with_paths=talloc_array(tmp_ctx, char *, n_files);
+
+  if (files_with_paths==NULL) {
+    tr_crit("dirent_to_full_path: unable to allocate filename array");
+    goto cleanup;
+  }
+
+  for (ii=0; ii<n_files; ii++) {
+    files_with_paths[ii]=join_paths(files_with_paths, dir, files[ii]->d_name);
+    if(files_with_paths[ii] == NULL) {
+      tr_crit("dirent_to_full_path: error joining path for %s.", files[ii]->d_name);
+      files_with_paths=NULL; /* will be freed automatically by talloc_free */
+      goto cleanup;
+    }
+  }
+
+cleanup:
+  if (files_with_paths!=NULL)
+    talloc_steal(mem_ctx, files_with_paths);
+  talloc_free(tmp_ctx);
+  return files_with_paths;
+}
+
+
 /* must specify the ctx and tr in cfgwatch! */
 int tr_read_and_apply_config(TR_CFGWATCH *cfgwatch)
 {
   TALLOC_CTX *tmp_ctx=talloc_new(NULL);
   char *config_dir=cfgwatch->config_dir;
-  int n_files = 0;
+  unsigned int n_files = 0;
   struct dirent **cfg_files=NULL;
   TR_CFG_RC rc = TR_CFG_SUCCESS;	/* presume success */
   struct tr_fstat *new_fstat_list=NULL;
+  char **files_with_paths=NULL;
   int retval=0;
 
   /* find the configuration files -- n.b., tr_find_config_files()
@@ -205,18 +253,26 @@ int tr_read_and_apply_config(TR_CFGWATCH *cfgwatch)
     tr_debug("tr_read_and_apply_config: No configuration files.");
     retval=1; goto cleanup;
   }
+  /* n_files > 0 from here on */
 
   /* Get the list of update times.
    * Do this before loading in case they change between obtaining their timestamp
    * and reading the file---this way they will immediately reload if this happens. */
-  new_fstat_list=tr_fstat_get_all(tmp_ctx, config_dir, cfg_files, n_files);
+  new_fstat_list=tr_fstat_get_all(tmp_ctx, config_dir, cfg_files, (unsigned int)n_files);
   if (new_fstat_list==NULL) {
     tr_debug("tr_read_and_apply_config: Could not allocate config file status list.");
     retval=1; goto cleanup;
   }
-  
+
+  /* get the filenames with their paths */
+  files_with_paths=dirent_to_full_path(tmp_ctx, config_dir, n_files, cfg_files);
+  if (files_with_paths==NULL) {
+    tr_err("tr_read_and_apply_config: Could not append path to filenames.");
+    retval=1; goto cleanup;
+  }
+
   /* now fill it in (tr_parse_config allocates space for new config) */
-  if (TR_CFG_SUCCESS != (rc = tr_parse_config(cfgwatch->cfg_mgr, config_dir, n_files, cfg_files))) {
+  if (TR_CFG_SUCCESS != (rc = tr_parse_config(cfgwatch->cfg_mgr, n_files, files_with_paths))) {
     tr_debug("tr_read_and_apply_config: Error parsing configuration information, rc=%d.", rc);
     retval=1; goto cleanup;
   }