fix(config): fix memory safety, memory leaks, and add unit test coverage

- Fix an out-of-bounds/segmentation fault in `config_getlist` when parsing
  command-line options that do not specify bracket offsets (e.g.
  `--rfsimulator.serveraddr`). Added index bracket verification and guarded
  `memcpy` against `NULL` pointers on list reallocation.
- Initialize `valid_idx` to `ParamList->numelt` in `config_getlist` to
  properly support overriding and appending elements when a configuration
  file has existing array items.
- Fix memory leaks in  `end_configmodule`
- Add  test cases in `test_config_cmdline.cpp`

Signed-off-by: Bartosz Podrygajlo <bartosz.podrygajlo@openairinterface.org> and assisted-by Gemini

common/CMakeLists.txt

@@ -2,6 +2,9 @@
 
 add_subdirectory(utils)
 add_subdirectory(config/yaml)
+if (ENABLE_TESTS)
+  add_subdirectory(config/tests)
+endif()
 
 configure_file(cmake_defs.h.in cmake_defs.h @ONLY)
 configure_file(oai_version.h.in oai_version.h @ONLY)

common/config/config_load_configmodule.c

@@ -417,12 +417,31 @@ void end_configmodule(configmodule_interface_t *cfgptr)
     
     cfgptr->numptrs=0;
     pthread_mutex_unlock(&cfgptr->memBlocks_mutex);
+    for (int i = 0; i < cfgptr->num_cfgP; i++) {
+      if (cfgptr->cfgP[i] != NULL) {
+        free(cfgptr->cfgP[i]);
+      }
+    }
     if (cfgptr->cfgmode)
       free(cfgptr->cfgmode);
 
     if (cfgptr->argv_info)
       free(cfgptr->argv_info);
 
+    if (cfgptr->tmpdir)
+      free(cfgptr->tmpdir);
+
+    if (cfgptr->status) {
+      if (cfgptr->status->debug_cfgname)
+        free(cfgptr->status->debug_cfgname);
+      free(cfgptr->status);
+    }
+
     free(cfgptr);
+    for (int i = 0; i < sizeofArray(Config_Params); i++) {
+      Config_Params[i].voidptr = NULL;
+      Config_Params[i].numelt = 0;
+      Config_Params[i].paramflags &= ~PARAMFLAG_PARAMSETDEF;
+    }
   }
 }

common/config/config_load_configmodule.h

@@ -48,8 +48,6 @@ typedef void (*configmodule_endfunc_t)(struct configmodule_interface *cfg);
 typedef int (*configmodule_initfunc_t)(struct configmodule_interface *cfg);
 
 typedef struct configmodule_status {
-  int num_paramgroups;
-  char **paramgroups_names;
   int num_err_nullvalue;
   int emptyla;
   int num_err_read;

common/config/config_userapi.c

@@ -159,6 +159,8 @@ int config_getlist(configmodule_interface_t *cfg, paramlist_def_t *ParamList, pa
       char *res = strstr(cfg->argv[i], searchstr);
       if (res != NULL) {
         char *bracket = strchr(res + strlen(searchstr), '[');
+        if (bracket == NULL)
+          continue;
         bracket++;
         long num = strtol(bracket, &endptr, 10);
         if (num < valid_idx)
@@ -180,7 +182,9 @@ int config_getlist(configmodule_interface_t *cfg, paramlist_def_t *ParamList, pa
       sprintf(cfgpath, "%s.[%i]", newprefix, new_idx);
       paramdef_t **old = ParamList->paramarray;
       ParamList->paramarray = config_allocate_new(cfg, (new_idx + 1) * sizeof(paramdef_t *), true);
-      memcpy(ParamList->paramarray, old, new_idx * sizeof(paramdef_t *));
+      if (new_idx > 0 && old != NULL) {
+        memcpy(ParamList->paramarray, old, new_idx * sizeof(paramdef_t *));
+      }
       ParamList->paramarray[new_idx] = config_allocate_new(cfg, numparams * sizeof(paramdef_t), true);
       memcpy(ParamList->paramarray[new_idx], params, sizeof(paramdef_t) * numparams);
 

common/config/tests/CMakeLists.txt

@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: LicenseRef-CSSL-1.0
+
+add_executable(test_config_cmdline test_config_cmdline.cpp)
+target_link_libraries(test_config_cmdline PRIVATE CONFIG_LIB GTest::gtest)
+add_dependencies(test_config_cmdline params_libconfig)
+add_dependencies(tests test_config_cmdline)
+
+configure_file(test_config.conf test_config.conf COPYONLY)
+
+add_test(NAME test_config_cmdline COMMAND ./test_config_cmdline)
+set_tests_properties(test_config_cmdline PROPERTIES ENVIRONMENT "LD_LIBRARY_PATH=${CMAKE_BINARY_DIR}")

common/config/tests/test_config.conf

@@ -0,0 +1,5 @@
+prefix = (
+  {
+    test = 42;
+  }
+);

common/config/tests/test_config_cmdline.cpp

@@ -0,0 +1,183 @@
+/*
+ * SPDX-License-Identifier: LicenseRef-CSSL-1.0
+ */
+
+#include "gtest/gtest.h"
+extern "C" {
+#include "common/config/config_userapi.h"
+#include "common/utils/LOG/log.h"
+void exit_function(const char *file, const char *function, const int line, const char *s, const int assert)
+{
+  if (assert) {
+    abort();
+  } else {
+    exit(EXIT_SUCCESS);
+  }
+}
+}
+
+configmodule_interface_t *uniqCfg = nullptr;
+
+void run_config_test(int argc, char **argv, int expected_numelt, int expected_ret)
+{
+  configmodule_interface_t *cfg = load_configmodule(argc, argv, CONFIG_ENABLECMDLINEONLY);
+  ASSERT_NE(cfg, nullptr);
+  uniqCfg = cfg;
+
+  paramdef_t params[] = {UINT16PARAM("test", "", 0, NULL, 0)};
+  paramlist_def_t list = {"prefix", NULL, 0};
+  int ret = config_getlist(cfg, &list, params, sizeofArray(params), NULL);
+
+  EXPECT_EQ(ret, expected_ret);
+  EXPECT_EQ(list.numelt, expected_numelt);
+
+  end_configmodule(cfg);
+  uniqCfg = nullptr;
+}
+
+TEST(cmdline, cmdline_new_array)
+{
+  char *array[] = {strdup("test_program"), strdup("--prefix.[0].test"), strdup("1")};
+  run_config_test(3, array, 1, 1);
+  for (auto i = 0U; i < sizeofArray(array); i++) {
+    free(array[i]);
+  }
+}
+
+TEST(cmdline, cmdline_no_crash_on_missing_bracket)
+{
+  char *array[] = {strdup("test_program"), strdup("--prefix.test"), strdup("1")};
+  run_config_test(3, array, 0, 0);
+  for (auto i = 0U; i < sizeofArray(array); i++) {
+    free(array[i]);
+  }
+}
+
+TEST(cmdline, cmdline_multiple_elements)
+{
+  char *array[] = {strdup("test_program"), strdup("--prefix.[0].test"), strdup("1"), strdup("--prefix.[1].test"), strdup("2")};
+  run_config_test(sizeofArray(array), array, 2, 2);
+  for (auto i = 0U; i < sizeofArray(array); i++) {
+    free(array[i]);
+  }
+}
+
+TEST(cmdline, cmdline_out_of_order_error)
+{
+  char *array[] = {strdup("test_program"), strdup("--prefix.[1].test"), strdup("1")};
+  run_config_test(sizeofArray(array), array, 0, -1);
+  for (auto i = 0U; i < sizeofArray(array); i++) {
+    free(array[i]);
+  }
+}
+
+TEST(cmdline, cmdline_reverse_order_error)
+{
+  char *array[] = {strdup("test_program"), strdup("--prefix.[1].test"), strdup("1"), strdup("--prefix.[0].test"), strdup("2")};
+  run_config_test(sizeofArray(array), array, 0, -1);
+  for (auto i = 0U; i < sizeofArray(array); i++) {
+    free(array[i]);
+  }
+}
+
+TEST(cmdline, load_file_and_cmdline)
+{
+  const char *conf_path = "test_config.conf";
+
+  char opt_string[256];
+  snprintf(opt_string, sizeof(opt_string), "%s", conf_path);
+
+  char *array[] = {strdup("test_program"), strdup("-O"), strdup(opt_string), strdup("--prefix.[1].test"), strdup("100")};
+
+  configmodule_interface_t *cfg = load_configmodule(sizeofArray(array), array, 0);
+  ASSERT_NE(cfg, nullptr);
+  uniqCfg = cfg;
+
+  paramdef_t params[] = {UINT16PARAM("test", "", 0, NULL, 0)};
+  paramlist_def_t list = {"prefix", NULL, 0};
+  int ret = config_getlist(cfg, &list, params, sizeofArray(params), NULL);
+
+  EXPECT_EQ(list.numelt, 2);
+  EXPECT_EQ(ret, 2);
+
+  ASSERT_NE(list.paramarray[0], nullptr);
+  ASSERT_NE(list.paramarray[0][0].u16ptr, nullptr);
+  EXPECT_EQ(*(list.paramarray[0][0].u16ptr), 42);
+
+  ASSERT_NE(list.paramarray[1], nullptr);
+  ASSERT_NE(list.paramarray[1][0].u16ptr, nullptr);
+  EXPECT_EQ(*(list.paramarray[1][0].u16ptr), 100);
+
+  end_configmodule(cfg);
+  uniqCfg = nullptr;
+
+  for (auto i = 0U; i < sizeofArray(array); i++) {
+    free(array[i]);
+  }
+}
+
+TEST(cmdline, load_file_and_overwrite)
+{
+  const char *conf_path = "test_config.conf";
+
+  char opt_string[256];
+  snprintf(opt_string, sizeof(opt_string), "%s", conf_path);
+
+  char *array[] = {strdup("test_program"), strdup("-O"), strdup(opt_string), strdup("--prefix.[0].test"), strdup("100")};
+
+  configmodule_interface_t *cfg = load_configmodule(5, array, 0);
+  ASSERT_NE(cfg, nullptr);
+  uniqCfg = cfg;
+
+  paramdef_t params[] = {UINT16PARAM("test", "", 0, NULL, 0)};
+  paramlist_def_t list = {"prefix", NULL, 0};
+  int ret = config_getlist(cfg, &list, params, sizeofArray(params), NULL);
+
+  EXPECT_EQ(list.numelt, 1);
+  EXPECT_EQ(ret, 1);
+
+  ASSERT_NE(list.paramarray[0], nullptr);
+  ASSERT_NE(list.paramarray[0][0].u16ptr, nullptr);
+  EXPECT_EQ(*(list.paramarray[0][0].u16ptr), 100);
+
+  end_configmodule(cfg);
+  uniqCfg = nullptr;
+
+  for (auto i = 0U; i < sizeofArray(array); i++) {
+    free(array[i]);
+  }
+}
+
+TEST(cmdline, load_file_and_out_of_order_error)
+{
+  const char *conf_path = "test_config.conf";
+
+  char opt_string[256];
+  snprintf(opt_string, sizeof(opt_string), "%s", conf_path);
+
+  char *array[] = {strdup("test_program"), strdup("-O"), strdup(opt_string), strdup("--prefix.[2].test"), strdup("100")};
+
+  configmodule_interface_t *cfg = load_configmodule(sizeofArray(array), array, 0);
+  ASSERT_NE(cfg, nullptr);
+  uniqCfg = cfg;
+
+  paramdef_t params[] = {UINT16PARAM("test", "", 0, NULL, 0)};
+  paramlist_def_t list = {"prefix", NULL, 0};
+  int ret = config_getlist(cfg, &list, params, sizeofArray(params), NULL);
+
+  EXPECT_EQ(ret, -1);
+
+  end_configmodule(cfg);
+  uniqCfg = nullptr;
+
+  for (auto i = 0U; i < sizeofArray(array); i++) {
+    free(array[i]);
+  }
+}
+
+int main(int argc, char **argv)
+{
+  logInit();
+  testing::InitGoogleTest(&argc, argv);
+  return RUN_ALL_TESTS();
+}