[ovs-dev] [PATCH v2] logical-fields: fix memory leak caused by initialize ovnfield_by_name twice

[Damijan Skvarc]

ovnfield_by_name is hash of strings which is used to quickly find
field by name. This hash is initialized from ovn_init_symtab(). In case
the latter function is called multiple times then also ovnfield_by_name is
initialized multiple times but without freeing previously allocated
memory resources what cause memory leaks.  This actually happens in
ovn-controller which calls ovn_init_symtab() function twice, once from
ofctrl.c and the other time from lflow.c files.

Problem was solved by initializing ovnfield_by_name entity only once
and using design pattern from stopwatch.c or meta_flow.c files (ovs).

Problem was reported by valgrind with flood of messages (190) while executing
ovn test suite:

    ==5999== 47 (32 direct, 15 indirect) bytes in 1 blocks are definitely lost in loss record 86 of 102
    ==5999==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==5999==    by 0x50635D: xmalloc (util.c:138)
    ==5999==    by 0x4F6513: shash_add_nocopy__ (shash.c:109)
    ==5999==    by 0x4F6585: shash_add_nocopy (shash.c:121)
    ==5999==    by 0x4F65BD: shash_add (shash.c:129)
    ==5999==    by 0x4F6602: shash_add_once (shash.c:136)
    ==5999==    by 0x4395B7: ovn_init_symtab (logical-fields.c:261)
    ==5999==    by 0x406C91: main (ovn-controller.c:1750)

Signed-off-by: Damijan Skvarc <damjan.skvarc at gmail.com>
---
 controller/lflow.c           |  3 +--
 include/ovn/logical-fields.h |  1 -
 lib/logical-fields.c         | 39 +++++++++++++++++++++++++++------------
 3 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index ee11fc6..143ea3c 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -827,7 +827,7 @@ lflow_handle_changed_neighbors(
     }
 }
 
-
+
 /* Translates logical flows in the Logical_Flow table in the OVN_SB database
  * into OpenFlow flows.  See ovn-architecture(7) for more information. */
 void
@@ -846,5 +846,4 @@ lflow_destroy(void)
 {
     expr_symtab_destroy(&symtab);
     shash_destroy(&symtab);
-    ovn_destroy_ovnfields();
 }
diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
index 9b7c34f..c7bd2db 100644
--- a/include/ovn/logical-fields.h
+++ b/include/ovn/logical-fields.h
@@ -130,5 +130,4 @@ ovn_field_from_id(enum ovn_field_id id)
 const char *event_to_string(enum ovn_controller_event event);
 int string_to_event(const char *s);
 const struct ovn_field *ovn_field_from_name(const char *name);
-void ovn_destroy_ovnfields(void);
 #endif /* ovn/lib/logical-fields.h */
diff --git a/lib/logical-fields.c b/lib/logical-fields.c
index 25ace58..a007085 100644
--- a/lib/logical-fields.c
+++ b/lib/logical-fields.c
@@ -254,12 +254,6 @@ ovn_init_symtab(struct shash *symtab)
     expr_symtab_add_field(symtab, "sctp.src", MFF_SCTP_SRC, "sctp", false);
     expr_symtab_add_field(symtab, "sctp.dst", MFF_SCTP_DST, "sctp", false);
 
-    shash_init(&ovnfield_by_name);
-    for (int i = 0; i < OVN_FIELD_N_IDS; i++) {
-        const struct ovn_field *of = &ovn_fields[i];
-        ovs_assert(of->id == i); /* Fields must be in the enum order. */
-        shash_add_once(&ovnfield_by_name, of->name, of);
-    }
     expr_symtab_add_ovn_field(symtab, "icmp4.frag_mtu", OVN_ICMP4_FRAG_MTU);
 }
 
@@ -284,14 +278,35 @@ string_to_event(const char *s)
     return -1;
 }
 
-const struct ovn_field *
-ovn_field_from_name(const char *name)
+static void
+ovn_destroy_ovnfields(void)
 {
-    return shash_find_data(&ovnfield_by_name, name);
+    shash_destroy(&ovnfield_by_name);
 }
 
-void
-ovn_destroy_ovnfields(void)
+static void
+ovn_do_init_ovnfields(void)
 {
-    shash_destroy(&ovnfield_by_name);
+    shash_init(&ovnfield_by_name);
+    for (int i = 0; i < OVN_FIELD_N_IDS; i++) {
+       const struct ovn_field *of = &ovn_fields[i];
+       ovs_assert(of->id == i); /* Fields must be in the enum order. */
+       shash_add_once(&ovnfield_by_name, of->name, of);
+    }
+    atexit(ovn_destroy_ovnfields);
+}
+
+static void
+ovn_init_ovnfields(void)
+{
+    static pthread_once_t once = PTHREAD_ONCE_INIT;
+    pthread_once(&once, ovn_do_init_ovnfields);
+}
+
+const struct ovn_field *
+ovn_field_from_name(const char *name)
+{
+    ovn_init_ovnfields();
+
+    return shash_find_data(&ovnfield_by_name, name);
 }
-- 
2.7.4