Skip to content

UCS/CONFIG: fixed crash on incorrect value set - #5987

Merged
yosefe merged 1 commit into
openucx:masterfrom
hoopoepg:topic/fixed-crash-on-incorrect-conf-value
Dec 9, 2020
Merged

yosefe merged 1 commit into
openucx:masterfrom
hoopoepg:topic/fixed-crash-on-incorrect-conf-value

Conversation

@hoopoepg

@hoopoepg hoopoepg commented Dec 4, 2020

Copy link
Copy Markdown
Contributor
  • there was possible double free of string when
    user tried to set incorrect value using ucs_config_parser_set_value
    function

fixes #5980

@yosefe yosefe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls add gtest

@hoopoepg

hoopoepg commented Dec 7, 2020

Copy link
Copy Markdown
Contributor Author

added test

@hoopoepg

hoopoepg commented Dec 7, 2020

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@yosefe

yosefe commented Dec 7, 2020

Copy link
Copy Markdown
Member

bot:pipe:retest

Comment thread test/gtest/ucs/test_config.cc Outdated
Comment on lines +486 to +487
scoped_log_handler log_handler_vars(config_error_suppress);
opts.set("COLOR", "magenta");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls wrap these in scope { }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrapped by {}

}

static ucs_log_func_rc_t
config_error_suppress(const char *file, unsigned line, const char *function,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can reuse wrap_errors_logger ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically yes, but in this case we can't check if error is handled - there is no errors counter in wrap_errors_logger

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we call wrap_errors_logger() after counting, to print err message?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, wrapped

Comment thread test/gtest/ucs/test_config.cc Outdated
Comment on lines +489 to +492
EXPECT_EQ(COLOR_WHITE, opts->color);
EXPECT_EQ(std::string(color_names[COLOR_WHITE]),
std::string(opts.get("COLOR")));
EXPECT_EQ(1, m_errors);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these can be outside the scope

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved outside of scope

Comment thread test/gtest/ucs/test_config.cc Outdated
m_errors++;
wrap_errors_logger(file, line, function, level, comp_conf,
message, ap);
return UCS_LOG_FUNC_RC_STOP;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: return wrap_errors_logger(..)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment thread test/gtest/ucs/test_config.cc Outdated
class test_config : public ucs::test {
public:
test_config() {
m_errors = 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: m_num_errors

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed to m_num_errors

@yosefe

yosefe commented Dec 9, 2020

Copy link
Copy Markdown
Member

@hoopoepg pls squash

- there was possible double free of string when
  user tried to set incorrect value using ucs_config_parser_set_value
  function
- added gtest
@hoopoepg
hoopoepg force-pushed the topic/fixed-crash-on-incorrect-conf-value branch from 529eaeb to 9d87dcb Compare December 9, 2020 10:47
@hoopoepg

hoopoepg commented Dec 9, 2020

Copy link
Copy Markdown
Contributor Author

squashed

@yosefe
yosefe merged commit 6294470 into openucx:master Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UCS: ucs_config_parser_set_value on incorrect value leads segmentation fault (double free)

2 participants