New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common: Passing null pointer option_name to operator << in md_config_t::parse_option() #15881

Merged
merged 1 commit into from Jun 27, 2017

Conversation

Projects
None yet
3 participants
@joscollin
Member

joscollin commented Jun 23, 2017

Fixes the Coverity Scan Report:

CID 1412776 (#1 of 1): Explicit null dereferenced (FORWARD_NULL)19. var_deref_model: Passing null pointer option_name to operator <<, which dereferences it.

Signed-off-by: Jos Collin jcollin@redhat.com

@joscollin

This comment has been minimized.

Member

joscollin commented Jun 24, 2017

Jenkins Retest this please

@@ -604,15 +604,19 @@ int md_config_t::parse_option(std::vector<const char*>& args,
if (ret != 0 || !error_message.empty()) {
if (oss) {
*oss << "Parse error setting " << option_name << " to '"
<< val << "' using injectargs";
if(option_name && !val.empty()) {

This comment has been minimized.

@tchaikov

tchaikov Jun 26, 2017

Contributor

under which circumstance, will option_name be null? and i think it's fine to print val even if it's empty.

This comment has been minimized.

@tchaikov

tchaikov Jun 26, 2017

Contributor

also, please add a space after if and before (.

This comment has been minimized.

@joscollin

joscollin Jun 26, 2017

Member

@tchaikov Yes you are right. It happens only if it doesn't enter the loop:

for (auto& opt_ref: *config_options) {

, which might be a rare case. But this check is good to silence the Coverity Warning and that's the main intention here.

This comment has been minimized.

@tchaikov

tchaikov Jun 26, 2017

Contributor

to silence the warning is the side effect, the point is to improve the quality of code, not to appease the static analyzer. if this does not happen, we should assert() it.

This comment has been minimized.

@joscollin

joscollin Jun 26, 2017

Member

@tchaikov Done. Please check.

@@ -604,13 +604,15 @@ int md_config_t::parse_option(std::vector<const char*>& args,
if (ret != 0 || !error_message.empty()) {
if (oss) {
assert(option_name);

This comment has been minimized.

@tchaikov

tchaikov Jun 26, 2017

Contributor

should lift the assert() out of the if clause.

This comment has been minimized.

@joscollin
common: Passing null pointer option_name to operator << in md_config_…
…t::parse_option()

Fixes the Coverity Scan Report:
CID 1412776 (#1 of 1): Explicit null dereferenced (FORWARD_NULL)19. var_deref_model: Passing null pointer option_name to operator <<, which dereferences it.

Fixed the review comments too in this commit.

Signed-off-by: Jos Collin <jcollin@redhat.com>

@tchaikov tchaikov added the needs-qa label Jun 26, 2017

@joscollin

This comment has been minimized.

Member

joscollin commented Jun 26, 2017

@tchaikov Thank you for your suggestions and for your effort.

@liewegas liewegas merged commit 6f6d6ed into ceph:master Jun 27, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
default Build finished.
Details
make check make check succeeded
Details

@joscollin joscollin deleted the joscollin:wip-1412776-explicit-null-dereferenced branch Jun 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment