Skip to content

Fix verbose command line option#2220

Merged
evgenyz merged 4 commits intoOpenSCAP:mainfrom
jan-cerny:verbose_option
Apr 15, 2025
Merged

Fix verbose command line option#2220
evgenyz merged 4 commits intoOpenSCAP:mainfrom
jan-cerny:verbose_option

Conversation

@jan-cerny
Copy link
Copy Markdown
Member

This commit ensures that the --verbose and --verbose-log-file options can be provided at any position on the command line and that they can be mixed between other options.

Addressing:
$ oscap xccdf eval --profile ospp --verbose DEVEL /usr/share/xml/scap/ssg/content/ssg-rhel8-ds.xml oscap: unrecognized option '--verbose'

Fixes: https://issues.redhat.com/browse/RHEL-53859

This commit ensures that the `--verbose` and `--verbose-log-file`
options can be provided at any position on the command line and
that they can be mixed between other options.

Addressing:
$ oscap xccdf eval --profile ospp --verbose DEVEL  /usr/share/xml/scap/ssg/content/ssg-rhel8-ds.xml
oscap: unrecognized option '--verbose'

Fixes: https://issues.redhat.com/browse/RHEL-53859
@jan-cerny jan-cerny added this to the 1.4.3 milestone Apr 10, 2025
local RF="results.xml"
local LOG="verbose.log"

$OSCAP xccdf --verbose=DEVEL eval --fetch-remote-resources --results $RF $DF 2>$LOG || echo "OK"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does the new option parsing mechanism fail for this option position? If not, why did you change it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, it now fails if the --verbose is between module and submodule name. Is it fine? The reason seems to be that the verbose options are now options of each submodule and not global options.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a backward-incompatible change.

@@ -776,19 +790,25 @@ bool getopt_info(int argc, char **argv, struct oscap_action *action)
while ((c = getopt_long(argc, argv, "o:i:p:", long_options, NULL)) != -1) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we still accept short options?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you mean here in oscap info module? The oscap info doesn't have any short options so I think we don't accept them.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Should the "o:i:p:" here be removed?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably. They won't work with your changes anyway.

The `optstring` parameter in `getopt_long` call in `oscap info`
submodule parser specifies some short options. However, the `oscap info`
submodule doesn't support any short options. It only supports long
options.  Therefore, these characters in the `optstring` parameter are
superfluous and should be removed.
@jan-cerny
Copy link
Copy Markdown
Member Author

I have removed them from the optstring and I have prevented the backward incompatible changes.

if (optind >= argc) {
return oscap_module_usage(action->module, stderr, "Wrong number of arguments!\n");
}
action->cpe_action = malloc(sizeof(struct cpe_action));
Copy link
Copy Markdown

@siteshwar siteshwar Apr 14, 2025

Choose a reason for hiding this comment

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

OpenScanHub reports several warnings around this allocation:

Error: GCC_ANALYZER_WARNING (CWE-476): [#def4]
openscap-1.4.3-build/openscap-1.4.3/utils/oscap-cpe.c: scope_hint: In function ‘getopt_cpe’
openscap-1.4.3-build/openscap-1.4.3/utils/oscap-cpe.c:134:42: warning[-Wanalyzer-possible-null-dereference]: dereference of possibly-NULL ‘*action.cpe_action’
#  132|   	action->cpe_action = malloc(sizeof(struct cpe_action));
#  133|   	if (action->module == &CPE_MATCH_MODULE) {
#  134|-> 		action->cpe_action->name = argv[optind];
#  135|   		action->cpe_action->dict = argv[optind + 1];
#  136|   	}

Error: GCC_ANALYZER_WARNING (CWE-476): [#def5]
openscap-1.4.3-build/openscap-1.4.3/utils/oscap-cpe.c:138:42: warning[-Wanalyzer-possible-null-dereference]: dereference of possibly-NULL ‘*action.cpe_action’
#  136|   	}
#  137|   	if (action->module == &CPE_CHECK_MODULE) {
#  138|-> 		action->cpe_action->name = argv[optind];
#  139|   	}
#  140|   	if (action->module == &CPE_VALIDATE) {

Error: GCC_ANALYZER_WARNING (CWE-476): [#def6]
openscap-1.4.3-build/openscap-1.4.3/utils/oscap-cpe.c:141:42: warning[-Wanalyzer-possible-null-dereference]: dereference of possibly-NULL ‘*action.cpe_action’
#  139|   	}
#  140|   	if (action->module == &CPE_VALIDATE) {
#  141|-> 		action->cpe_action->dict = argv[optind];
#  142|   	}
#  143|   	return true;

Please feel free to fix them if they may be real issues. Thanks!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍

@jan-cerny
Copy link
Copy Markdown
Member Author

I have add ensure cpe_action is not NULL

Copy link
Copy Markdown
Contributor

@evgenyz evgenyz left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@evgenyz evgenyz merged commit 3cf3ba2 into OpenSCAP:main Apr 15, 2025
18 checks passed
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.

3 participants