Merge pull request #5987 from hoopoepg/topic/fixed-crash-on-incorrect…

…-conf-value UCS/CONFIG: fixed crash on incorrect value set
openucx · Dec 9, 2020 · 6294470 · 6294470
2 parents f9c253a + 9d87dcb
commit 6294470
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 0 deletions.
diff --git a/src/ucs/config/parser.c b/src/ucs/config/parser.c
@@ -1033,9 +1033,12 @@ ucs_config_parser_set_value_internal(void *opts, ucs_config_field_t *fields,
                                      const char *name, const char *value,
                                      const char *table_prefix, int recurse)
 {
+    char value_buf[256] = "";
     ucs_config_field_t *field, *sub_fields;
     size_t prefix_len;
     ucs_status_t status;
+    ucs_status_t UCS_V_UNUSED status_restore;
+    int UCS_V_UNUSED ret;
     unsigned count;
     void *var;
 
@@ -1079,9 +1082,17 @@ ucs_config_parser_set_value_internal(void *opts, ucs_config_field_t *fields,
                 return UCS_ERR_NO_ELEM;
             }
 
+            /* backup current value to restore it in case if new value
+             * is not accepted */
+            ret = field->parser.write(value_buf, sizeof(value_buf) - 1, var,
+                                      field->parser.arg);
+            ucs_assert(ret != 0); /* write success */
             ucs_config_parser_release_field(field, var);
             status = ucs_config_parser_parse_field(field, value, var);
             if (status != UCS_OK) {
+                status_restore = ucs_config_parser_parse_field(field, value_buf, var);
+                /* current value must be valid */
+                ucs_assert(status_restore == UCS_OK);
                 return status;
             }
             ++count;

diff --git a/test/gtest/ucs/test_config.cc b/test/gtest/ucs/test_config.cc
@@ -224,7 +224,14 @@ ucs_config_field_t car_opts_table[] = {
 static std::vector<std::string> config_err_exp_str;
 
 class test_config : public ucs::test {
+public:
+    test_config() {
+        m_num_errors = 0;
+    }
+
 protected:
+    static int m_num_errors;
+
     static ucs_log_func_rc_t
     config_error_handler(const char *file, unsigned line, const char *function,
                          ucs_log_level_t level,
@@ -246,6 +253,22 @@ class test_config : public ucs::test {
         return UCS_LOG_FUNC_RC_CONTINUE;
     }
 
+    static ucs_log_func_rc_t
+    config_error_suppress(const char *file, unsigned line, const char *function,
+                          ucs_log_level_t level,
+                          const ucs_log_component_config_t *comp_conf,
+                          const char *message, va_list ap)
+    {
+        // Ignore errors that invalid input parameters as it is expected
+        if (level == UCS_LOG_LEVEL_ERROR) {
+            m_num_errors++;
+            return wrap_errors_logger(file, line, function, level, comp_conf,
+                                      message, ap);
+        }
+
+        return UCS_LOG_FUNC_RC_CONTINUE;
+    }
+
     /*
      * Wrapper class for car options parser.
      */
@@ -377,6 +400,8 @@ class test_config : public ucs::test {
     }
 };
 
+int test_config::m_num_errors;
+
 UCS_TEST_F(test_config, parse_default) {
     car_opts opts(UCS_DEFAULT_ENV_PREFIX, "TEST");
 
@@ -457,6 +482,17 @@ UCS_TEST_F(test_config, set_get) {
 
     opts.set("VIN", "123456");
     EXPECT_EQ(123456UL, opts->vin);
+
+    /* try to set incorrect value - color should not be updated */
+    {
+        scoped_log_handler log_handler_vars(config_error_suppress);
+        opts.set("COLOR", "magenta");
+    }
+
+    EXPECT_EQ(COLOR_WHITE, opts->color);
+    EXPECT_EQ(std::string(color_names[COLOR_WHITE]),
+            std::string(opts.get("COLOR")));
+    EXPECT_EQ(1, m_num_errors);
 }
 
 UCS_TEST_F(test_config, set_get_with_table_prefix) {