Conf: Fixes bug in symbol lookup during reconfiguration

Symbol lookup by cf_find_symbol() not only did the lookup but also added
new void symbols allocated from cfg_mem linpool, which gets broken when
lookups are done outside of config parsing, which may lead to crashes
during reconfiguration.

The patch separates lookup-only cf_find_symbol() and config-modifying
cf_get_symbol(), while the later is called only during parsing. Also
new_config and cfg_mem global variables are NULLed outside of parsing.

diff --git a/conf/cf-lex.l b/conf/cf-lex.l
index 61e0407585dec08d1589ce3ec628a5a5b34766e5..5a2a4d6b6e3e77bfb43530e52675d832b1cd0f30 100644 (file)
--- a/conf/cf-lex.l
+++ b/conf/cf-lex.l
@@ -70,7 +70,7 @@ struct sym_scope {
 static struct sym_scope *conf_this_scope;
 
 static int cf_hash(byte *c);
-static struct symbol *cf_find_sym(byte *c, unsigned int h0);
+static inline struct symbol * cf_get_sym(byte *c, uint h0);
 
 linpool *cfg_mem;
 
@@ -194,7 +194,7 @@ else: {
        }
       k=k->next;
     }
-  cf_lval.s = cf_find_sym(yytext, h);
+  cf_lval.s = cf_get_sym(yytext, h);
   return SYM;
 }
 
@@ -426,8 +426,9 @@ check_eof(void)
 }
 
 static struct symbol *
-cf_new_sym(byte *c, unsigned int h)
+cf_new_sym(byte *c, uint h0)
 {
+  uint h = h0 & (SYM_HASH_SIZE-1);
   struct symbol *s, **ht;
   int l;
 
@@ -449,56 +450,77 @@ cf_new_sym(byte *c, unsigned int h)
 }
 
 static struct symbol *
-cf_find_sym(byte *c, unsigned int h0)
+cf_find_sym(struct config *cfg, byte *c, uint h0)
 {
-  unsigned int h = h0 & (SYM_HASH_SIZE-1);
+  uint h = h0 & (SYM_HASH_SIZE-1);
   struct symbol *s, **ht;
 
-  if (ht = new_config->sym_hash)
+  if (ht = cfg->sym_hash)
     {
       for(s = ht[h]; s; s=s->next)
        if (!strcmp(s->name, c) && s->scope->active)
          return s;
     }
-  if (new_config->sym_fallback)
+  if (ht = cfg->sym_fallback)
     {
       /* We know only top-level scope is active */
-      for(s = new_config->sym_fallback[h]; s; s=s->next)
+      for(s = ht[h]; s; s=s->next)
        if (!strcmp(s->name, c) && s->scope->active)
          return s;
     }
-  return cf_new_sym(c, h);
+
+  return NULL;
+}
+
+static inline struct symbol *
+cf_get_sym(byte *c, uint h0)
+{
+  return cf_find_sym(new_config, c, h0) ?: cf_new_sym(c, h0);
 }
 
 /**
  * cf_find_symbol - find a symbol by name
+ * @cfg: specificed config
+ * @c: symbol name
+ *
+ * This functions searches the symbol table in the config @cfg for a symbol of
+ * given name. First it examines the current scope, then the second recent one
+ * and so on until it either finds the symbol and returns a pointer to its
+ * &symbol structure or reaches the end of the scope chain and returns %NULL to
+ * signify no match.
+ */
+struct symbol *
+cf_find_symbol(struct config *cfg, byte *c)
+{
+  return cf_find_sym(cfg, c, cf_hash(c));
+}
+
+/**
+ * cf_get_symbol - get a symbol by name
  * @c: symbol name
  *
- * This functions searches the symbol table for a symbol of given
- * name. First it examines the current scope, then the second recent
- * one and so on until it either finds the symbol and returns a pointer
- * to its &symbol structure or reaches the end of the scope chain
- * and returns %NULL to signify no match.
+ * This functions searches the symbol table of the currently parsed config
+ * (@new_config) for a symbol of given name. It returns either the already
+ * existing symbol or a newly allocated undefined (%SYM_VOID) symbol if no
+ * existing symbol is found.
  */
 struct symbol *
-cf_find_symbol(byte *c)
+cf_get_symbol(byte *c)
 {
-  return cf_find_sym(c, cf_hash(c));
+  return cf_get_sym(c, cf_hash(c));
 }
 
 struct symbol *
 cf_default_name(char *template, int *counter)
 {
-  char buf[32];
+  char buf[SYM_MAX_LEN];
   struct symbol *s;
   char *perc = strchr(template, '%');
 
   for(;;)
     {
       bsprintf(buf, template, ++(*counter));
-      s = cf_find_sym(buf, cf_hash(buf));
-      if (!s)
-       break;
+      s = cf_get_sym(buf, cf_hash(buf));
       if (s->class == SYM_VOID)
        return s;
       if (!perc)
@@ -529,7 +551,7 @@ cf_define_symbol(struct symbol *sym, int type, void *def)
     {
       if (sym->scope == conf_this_scope)
        cf_error("Symbol already defined");
-      sym = cf_new_sym(sym->name, cf_hash(sym->name) & (SYM_HASH_SIZE-1));
+      sym = cf_new_sym(sym->name, cf_hash(sym->name));
     }
   sym->class = type;
   sym->def = def;

diff --git a/conf/conf.c b/conf/conf.c
index a907402d936ef34062326ce65b196213de7ce8f8..825a8e9f161e3861e7e15d016ef5e914237131c4 100644 (file)
--- a/conf/conf.c
+++ b/conf/conf.c
@@ -20,19 +20,19 @@
  *
  * There can exist up to four different configurations at one time: an active
  * one (pointed to by @config), configuration we are just switching from
- * (@old_config), one queued for the next reconfiguration (@future_config;
- * if there is one and the user wants to reconfigure once again, we just
- * free the previous queued config and replace it with the new one) and
- * finally a config being parsed (@new_config). The stored @old_config 
- * is also used for undo reconfiguration, which works in a similar way.
- * Reconfiguration could also have timeout (using @config_timer) and undo
- * is automatically called if the new configuration is not confirmed later.
+ * (@old_config), one queued for the next reconfiguration (@future_config; if
+ * there is one and the user wants to reconfigure once again, we just free the
+ * previous queued config and replace it with the new one) and finally a config
+ * being parsed (@new_config). The stored @old_config is also used for undo
+ * reconfiguration, which works in a similar way. Reconfiguration could also
+ * have timeout (using @config_timer) and undo is automatically called if the
+ * new configuration is not confirmed later. The new config (@new_config) and
+ * associated linear pool (@cfg_mem) is non-NULL only during parsing.
  *
- * Loading of new configuration is very simple: just call config_alloc()
- * to get a new &config structure, then use config_parse() to parse a
- * configuration file and fill all fields of the structure
- * and finally ask the config manager to switch to the new
- * config by calling config_commit().
+ * Loading of new configuration is very simple: just call config_alloc() to get
+ * a new &config structure, then use config_parse() to parse a configuration
+ * file and fill all fields of the structure and finally ask the config manager
+ * to switch to the new config by calling config_commit().
  *
  * CLI commands are parsed in a very similar way -- there is also a stripped-down
  * &config structure associated with them and they are lex-ed and parsed by the
@@ -91,10 +91,15 @@ config_alloc(byte *name)
   linpool *l = lp_new(p, 4080);
   struct config *c = lp_allocz(l, sizeof(struct config));
 
+  /* Duplication of name string in local linear pool */
+  uint nlen = strlen(name) + 1;
+  char *ndup = lp_allocu(l, nlen);
+  memcpy(ndup, name, nlen);
+
   c->mrtdump_file = -1; /* Hack, this should be sysdep-specific */
   c->pool = p;
-  cfg_mem = c->mem = l;
-  c->file_name = cfg_strdup(name);
+  c->mem = l;
+  c->file_name = ndup;
   c->load_time = now;
   c->tf_route = c->tf_proto = (struct timeformat){"%T", "%F", 20*3600};
   c->tf_base = c->tf_log = (struct timeformat){"%F %T", NULL, 0};
@@ -119,11 +124,13 @@ config_alloc(byte *name)
 int
 config_parse(struct config *c)
 {
+  int done = 0;
   DBG("Parsing configuration file `%s'\n", c->file_name);
   new_config = c;
   cfg_mem = c->mem;
   if (setjmp(conf_jmpbuf))
-    return 0;
+    goto cleanup;
+
   cf_lex_init(0, c);
   sysdep_preconfig(c);
   protos_preconfig(c);
@@ -137,7 +144,12 @@ config_parse(struct config *c)
   if (!c->router_id)
     cf_error("Router ID must be configured manually on IPv6 routers");
 #endif
-  return 1;
+  done = 1;
+
+cleanup:
+  new_config = NULL;
+  cfg_mem = NULL;
+  return done;
 }
 
 /**
@@ -150,14 +162,22 @@ config_parse(struct config *c)
 int
 cli_parse(struct config *c)
 {
-  new_config = c;
+  int done = 0;
   c->sym_fallback = config->sym_hash;
+  new_config = c;
   cfg_mem = c->mem;
   if (setjmp(conf_jmpbuf))
-    return 0;
+    goto cleanup;
+
   cf_lex_init(1, c);
   cf_parse();
-  return 1;
+  done = 1;
+
+cleanup:
+  c->sym_fallback = NULL;
+  new_config = NULL;
+  cfg_mem = NULL;
+  return done;
 }
 
 /**
@@ -237,10 +257,6 @@ config_do_commit(struct config *c, int type)
   if (old_config && !config->shutdown)
     log(L_INFO "Reconfiguring");
 
-  /* This should not be necessary, but it seems there are some
-     functions that access new_config instead of config */
-  new_config = config;
-
   if (old_config)
     old_config->obstacle_count++;
 
@@ -254,9 +270,6 @@ config_do_commit(struct config *c, int type)
   DBG("protos_commit\n");
   protos_commit(c, old_config, force_restart, type);
 
-  /* Just to be sure nobody uses that now */
-  new_config = NULL;
-
   int obs = 0;
   if (old_config)
     obs = --old_config->obstacle_count;

diff --git a/conf/conf.h b/conf/conf.h
index 515efbb36146d146c075279d090cfd2daf5734cd..89a2c5b715261d89cad0201484681866b9d5e1ee 100644 (file)
--- a/conf/conf.h
+++ b/conf/conf.h
@@ -147,7 +147,9 @@ int cf_lex(void);
 void cf_lex_init(int is_cli, struct config *c);
 void cf_lex_unwind(void);
 
-struct symbol *cf_find_symbol(byte *c);
+struct symbol *cf_find_symbol(struct config *cfg, byte *c);
+
+struct symbol *cf_get_symbol(byte *c);
 struct symbol *cf_default_name(char *template, int *counter);
 struct symbol *cf_define_symbol(struct symbol *symbol, int type, void *def);
 void cf_push_scope(struct symbol *);

diff --git a/nest/proto.c b/nest/proto.c
index 6531083cf804d9027e470b8211fb086bdaced45c..d04da333c6731809acabb21dce1e834ac64b2181 100644 (file)
--- a/nest/proto.c
+++ b/nest/proto.c
@@ -521,7 +521,7 @@ protos_commit(struct config *new, struct config *old, int force_reconfig, int ty
       WALK_LIST(oc, old->protos)
        {
          p = oc->proto;
-         sym = cf_find_symbol(oc->name);
+         sym = cf_find_symbol(new, oc->name);
          if (sym && sym->class == SYM_PROTO && !new->shutdown)
            {
              /* Found match, let's check if we can smoothly switch to new configuration */

diff --git a/nest/rt-roa.c b/nest/rt-roa.c
index aa453f16f57f381922629b57e5763bb9d291d8b2..0fd89667201d29340682cfbebe921cb43e811ec6 100644 (file)
--- a/nest/rt-roa.c
+++ b/nest/rt-roa.c
@@ -311,7 +311,7 @@ roa_commit(struct config *new, struct config *old)
   if (old)
     WALK_LIST(t, roa_table_list)
       {
-       struct symbol *sym = cf_find_symbol(t->name);
+       struct symbol *sym = cf_find_symbol(new, t->name);
        if (sym && sym->class == SYM_ROA)
          {
            /* Found old table in new config */

diff --git a/nest/rt-table.c b/nest/rt-table.c
index 9e2c4e0ddc2443171cf9704195a28c252e035a6d..2ddff12eae465f01fe1b38b25cf22b20ee9a20e6 100644 (file)
--- a/nest/rt-table.c
+++ b/nest/rt-table.c
@@ -1663,7 +1663,7 @@ rt_prune_loop(void)
 void
 rt_preconfig(struct config *c)
 {
-  struct symbol *s = cf_find_symbol("master");
+  struct symbol *s = cf_get_symbol("master");
 
   init_list(&c->tables);
   c->master_rtc = rt_new_table(s);
@@ -1903,7 +1903,7 @@ rt_commit(struct config *new, struct config *old)
          rtable *ot = o->table;
          if (!ot->deleted)
            {
-             struct symbol *sym = cf_find_symbol(o->name);
+             struct symbol *sym = cf_find_symbol(new, o->name);
              if (sym && sym->class == SYM_TABLE && !new->shutdown)
                {
                  DBG("\t%s: same\n", o->name);

diff --git a/sysdep/unix/main.c b/sysdep/unix/main.c
index e31471da3fa33120fa344304b592e561145b2863..24d34f6017e1a37b1b2442eef2ae2e982f4f12ee 100644 (file)
--- a/sysdep/unix/main.c
+++ b/sysdep/unix/main.c
@@ -96,7 +96,7 @@ drop_gid(gid_t gid)
 static inline void
 add_num_const(char *name, int val)
 {
-  struct symbol *s = cf_find_symbol(name);
+  struct symbol *s = cf_get_symbol(name);
   s->class = SYM_CONSTANT | T_INT;
   s->def = cfg_allocz(sizeof(struct f_val));
   SYM_TYPE(s) = T_INT;