From 0bb4f170d27fcec00998aec5def2b048210cae93 Mon Sep 17 00:00:00 2001 From: Danilo Egea Gondolfo Date: Wed, 19 Jul 2023 22:27:57 +0100 Subject: [PATCH] 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. --- src/parse-nm.c | 24 ++++++-------------- tests/ctests/test_netplan_keyfile.c | 34 +++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/src/parse-nm.c b/src/parse-nm.c index 9bfb96212..a6d4e3e07 100644 --- 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 index a3e19b705..ae36ee55c 100644 --- 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);