parse-nm: plug a memory leak

The variable options_kf_value would leak if a route_options key is found without a route in the keyfile, the loop is being interrupted before the variable is free'd. Add a test that causes the problem. Found by Coverity.
canonical · Jul 19, 2023 · 0bb4f17 · 0bb4f17
1 parent a771f0e
commit 0bb4f17
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 17 deletions.
diff --git a/src/parse-nm.c b/src/parse-nm.c
@@ -223,23 +223,17 @@ parse_routes(GKeyFile* kf, const gchar* group, GArray** routes_arr)
 {
     g_assert(routes_arr);
     NetplanIPRoute *route = NULL;
-    gchar *key = NULL;
-    gchar *kf_value = NULL;
-    gchar *options_key = NULL;
-    gchar *options_kf_value = NULL;
     gchar **split = NULL;
     for (unsigned i = 1;; ++i) {
         gboolean unhandled_data = FALSE;
-        key = g_strdup_printf("route%u", i);
-        kf_value = g_key_file_get_string(kf, group, key, NULL);
-        options_key = g_strdup_printf("route%u_options", i);
-        options_kf_value = g_key_file_get_string(kf, group, options_key, NULL);
-        if (!options_kf_value)
-            g_free(options_key);
-        if (!kf_value) {
-            g_free(key);
+        g_autofree gchar* key = g_strdup_printf("route%u", i);
+        g_autofree gchar* kf_value = g_key_file_get_string(kf, group, key, NULL);
+        g_autofree gchar* options_key = g_strdup_printf("route%u_options", i);
+        g_autofree gchar* options_kf_value = g_key_file_get_string(kf, group, options_key, NULL);
+
+        if (!kf_value)
             break;
-        }
+
         if (!*routes_arr)
             *routes_arr = g_array_new(FALSE, TRUE, sizeof(NetplanIPRoute*));
         route = g_new0(NetplanIPRoute, 1);
@@ -303,16 +297,12 @@ parse_routes(GKeyFile* kf, const gchar* group, GArray** routes_arr)
 
             if (!unhandled_data)
                 _kf_clear_key(kf, group, options_key);
-            g_free(options_key);
-            g_free(options_kf_value);
         }
 
         /* Add route to array, clear keyfile */
         g_array_append_val(*routes_arr, route);
         if (!unhandled_data)
             _kf_clear_key(kf, group, key);
-        g_free(key);
-        g_free(kf_value);
     }
 }
 

diff --git a/tests/ctests/test_netplan_keyfile.c b/tests/ctests/test_netplan_keyfile.c
@@ -213,7 +213,9 @@ test_load_keyfile_multiple_addresses_and_routes(__unused void** state)
         "address1=10.100.1.38/24\n"
         "address2=10.100.1.39/24\n"
         "route1=0.0.0.0/0,10.100.1.1\n"
+        "route1_options=onlink=true,initrwnd=33,initcwnd=44,mtu=1024,table=102\n"
         "route2=192.168.0.0/24,1.2.3.4\n"
+        "route2_options=onlink=true,initrwnd=33,initcwnd=44,mtu=1024,table=103\n"
         "[ipv6]\n"
         "method=manual\n"
         "address1=2001:cafe:face::1/64\n"
@@ -231,6 +233,37 @@ test_load_keyfile_multiple_addresses_and_routes(__unused void** state)
     netplan_state_clear(&np_state);
 }
 
+void
+test_load_keyfile_route_options_without_route(__unused void** state)
+{
+    NetplanState *np_state = NULL;
+    NetplanStateIterator iter;
+    NetplanNetDefinition* netdef = NULL;
+
+    /* Should this even be allowed? */
+    const char* keyfile =
+        "[connection]\n"
+        "id=netplan-enp3s0\n"
+        "type=ethernet\n"
+        "interface-name=enp3s0\n"
+        "uuid=6352c897-174c-4f61-9623-556eddad05b2\n"
+        "[ipv4]\n"
+        "method=manual\n"
+        "address1=10.100.1.38/24\n"
+        "address2=10.100.1.39/24\n"
+        "route1_options=onlink=true,initrwnd=33,initcwnd=44,mtu=1024,table=102,src=10.10.10.11\n";
+
+    np_state = load_keyfile_string_to_netplan_state(keyfile);
+
+    netplan_state_iterator_init(np_state, &iter);
+    netdef = netplan_state_iterator_next(&iter);
+
+    assert_string_equal(netdef->id, "NM-6352c897-174c-4f61-9623-556eddad05b2");
+
+    netplan_state_clear(&np_state);
+}
+
+
 int
 setup(__unused void** state)
 {
@@ -254,6 +287,7 @@ main()
         cmocka_unit_test(test_load_keyfile_wireguard_with_bad_peer_key),
         cmocka_unit_test(test_load_keyfile_vxlan),
         cmocka_unit_test(test_load_keyfile_multiple_addresses_and_routes),
+        cmocka_unit_test(test_load_keyfile_route_options_without_route),
     };
 
     return cmocka_run_group_tests(tests, setup, tear_down);