dfilter: fix memleaks with functions and slice operator
authorPeter Wu <peter@lekensteyn.nl>
Tue, 24 Apr 2018 20:34:26 +0000 (22:34 +0200)
committerAnders Broman <a.broman58@gmail.com>
Wed, 25 Apr 2018 06:57:00 +0000 (06:57 +0000)
Running tools/dfilter-test.py with LSan enabled resulted in 38 test
failures due to memory leaks from "fvalue_new". Problematic dfilters:
- Return values from functions, e.g. `len(data.data) > 8` (instruction
  CALL_FUNCTION invoking functions from epan/dfilter/dfunctions.c)
- Slice operator: `data.data[1:2] == aa:bb` (function mk_range)

These values end up in "registers", but as some values (from READ_TREE)
reference the proto tree, a new tracking flag ("owns_memory") is added.

Add missing tests for some functions and try to improve documentation.

Change-Id: I28e8cf872675d0a81ea7aa5fac7398257de3f47b
Reviewed-on: https://code.wireshark.org/review/27132
Petri-Dish: Peter Wu <peter@lekensteyn.nl>
Reviewed-by: Peter Wu <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>
epan/dfilter/dfilter-int.h
epan/dfilter/dfilter.c
epan/dfilter/dfvm.c
tools/dftestlib/ipv4.py
tools/dftestlib/string_type.py
tshark.c

index 785f5a33153aaf1c1ab19d4f909e2eedf90d9ef8..9d53353954d812ad3799c4306b9b0148a55586c3 100644 (file)
@@ -23,6 +23,7 @@ struct epan_dfilter {
        guint           max_registers;
        GList           **registers;
        gboolean        *attempted_load;
+       gboolean        *owns_memory;
        int             *interesting_fields;
        int             num_interesting_fields;
        GPtrArray       *deprecated;
index a975f84e849f44f80c86726198dee24b31e4d067..c323ca5266c2de0e8b846d7c3a7a33ee0a94c9f7 100644 (file)
@@ -133,11 +133,10 @@ dfilter_free(dfilter_t *df)
 
        g_free(df->interesting_fields);
 
-       /* clear registers */
-       for (i = 0; i < df->max_registers; i++) {
-               if (df->registers[i]) {
-                       g_list_free(df->registers[i]);
-               }
+       /* Clear registers with constant values (as set by dfvm_init_const).
+        * Other registers were cleared on RETURN by free_register_overhead. */
+       for (i = df->num_registers; i < df->max_registers; i++) {
+               g_list_free(df->registers[i]);
        }
 
        if (df->deprecated) {
@@ -150,6 +149,7 @@ dfilter_free(dfilter_t *df)
 
        g_free(df->registers);
        g_free(df->attempted_load);
+       g_free(df->owns_memory);
        g_free(df);
 }
 
@@ -350,6 +350,7 @@ dfilter_compile(const gchar *text, dfilter_t **dfp, gchar **err_msg)
                dfilter->max_registers = dfw->next_register;
                dfilter->registers = g_new0(GList*, dfilter->max_registers);
                dfilter->attempted_load = g_new0(gboolean, dfilter->max_registers);
+               dfilter->owns_memory = g_new0(gboolean, dfilter->max_registers);
 
                /* Initialize constants */
                dfvm_init_const(dfilter);
index f93fd50edf69da2ce4abf9f3f67c1dd1f156e5a1..322ca0581228f685c47257ada5ec783c116b41b2 100644 (file)
@@ -331,14 +331,19 @@ read_tree(dfilter_t *df, proto_tree *tree, header_field_info *hfinfo, int reg)
        }
 
        df->registers[reg] = fvalues;
+       // These values are referenced only, do not try to free it later.
+       df->owns_memory[reg] = FALSE;
        return TRUE;
 }
 
 
+/* Put a constant value in a register. These will not be cleared by
+ * free_register_overhead. */
 static gboolean
 put_fvalue(dfilter_t *df, fvalue_t *fv, int reg)
 {
        df->registers[reg] = g_list_append(NULL, fv);
+       df->owns_memory[reg] = FALSE;
        return TRUE;
 }
 
@@ -396,8 +401,15 @@ any_in_range(dfilter_t *df, int reg1, int reg2, int reg3)
 }
 
 
-/* Free the list nodes w/o freeing the memory that each
- * list node points to. */
+static void
+free_owned_register(gpointer data, gpointer user_data _U_)
+{
+       fvalue_t *value = (fvalue_t *)data;
+       FVALUE_FREE(value);
+}
+
+/* Clear registers that were populated during evaluation (leaving constants
+ * intact). If we created the values, then these will be freed as well. */
 static void
 free_register_overhead(dfilter_t* df)
 {
@@ -406,6 +418,10 @@ free_register_overhead(dfilter_t* df)
        for (i = 0; i < df->num_registers; i++) {
                df->attempted_load[i] = FALSE;
                if (df->registers[i]) {
+                       if (df->owns_memory[i]) {
+                               g_list_foreach(df->registers[i], free_owned_register, NULL);
+                               df->owns_memory[i] = FALSE;
+                       }
                        g_list_free(df->registers[i]);
                        df->registers[i] = NULL;
                }
@@ -437,6 +453,7 @@ mk_range(dfilter_t *df, int from_reg, int to_reg, drange_t *d_range)
        }
 
        df->registers[to_reg] = to_list;
+       df->owns_memory[to_reg] = TRUE;
 }
 
 
@@ -499,6 +516,8 @@ dfvm_apply(dfilter_t *df, proto_tree *tree)
                                }
                                accum = arg1->value.funcdef->function(param1, param2,
                                                &df->registers[arg2->value.numeric]);
+                               // functions create a new value, so own it.
+                               df->owns_memory[arg2->value.numeric] = TRUE;
                                break;
 
                        case MK_RANGE:
index 1ed5cf45ccc8a7c7b4f67bcd0c49f427c350306d..c32e7fbf945dd6898f1edafed9fe7de03faa4e1a 100644 (file)
@@ -123,3 +123,11 @@ class testIPv4(dftest.DFTest):
     def test_slice_4(self):
          dfilter = "ip.src[2:2] == ff:ff"
          self.assertDFilterCount(dfilter, 0)
+
+    def test_count_1(self):
+         dfilter = "count(ip.src) == 1"
+         self.assertDFilterCount(dfilter, 2)
+
+    def test_count_2(self):
+         dfilter = "count(ip.addr) == 2"
+         self.assertDFilterCount(dfilter, 2)
index b5573cdda963e5511f83f6fe1d166a6df78caa7a..8428469a43bea88ef804c95dec2d471627a86446 100644 (file)
@@ -156,7 +156,10 @@ class testString(dftest.DFTest):
         dfilter = 'lower(http.user_agent) contains "update"'
         self.assertDFilterCount(dfilter, 1)
 
-    def test_contains_lower_2(self):
+    def test_eq_lower_1(self):
         dfilter = 'lower(tcp.seq) == 4'
         self.assertDFilterFail(dfilter)
 
+    def test_string_len(self):
+        dfilter = 'len(http.request.method) == 4'
+        self.assertDFilterCount(dfilter, 1)
index 504aae4226f6cc01e8a8af8d0a946be05481ceff..fec5266d633c0dedaf1044978607ee892779aebb 100644 (file)
--- a/tshark.c
+++ b/tshark.c
@@ -2249,6 +2249,7 @@ clean_exit:
   wtap_cleanup();
   free_progdirs();
   cf_close(&cfile);
+  dfilter_free(dfcode);
   return exit_status;
 }