Fix CMP CLI tests by DDvO · Pull Request #12175 · openssl/openssl

DDvO · 2020-06-17T06:15:56Z

Fix CMP -days option range checking and test failing with enable-ubsan, reported by @romen Chunk 12 of CMP contribution to OpenSSL: CLI-based test #11998 (comment)
Fix Master fails "81-test_cmp.cli.t" #12156 by doing connections to 127.0.0.1 (e.g., Mock server) without proxy
Fix Extended tests are failing cmp tests #12145 by disabling some test in case of fuzzing
etc. (see my Fix CMP CLI tests #12175 (comment) further below)

slontis · 2020-06-17T23:18:25Z

Is this still WIP?

DDvO · 2020-06-18T05:01:09Z

Is this still WIP?

~~Yes. Currently I'm waiting on further input on #12145.~~

DDvO · 2020-06-18T05:47:17Z

I think I've meanwhile fixed all reported issues.

DDvO · 2020-06-18T05:48:20Z

Is this still WIP?

Not anymore.

crypto/cmp/cmp_ctx.c

doc/man3/OSSL_CRMF_MSG_set0_validity.pod

DDvO · 2020-06-20T15:28:13Z

I've just added three more fixes:

Speed-up for tests in 81-test_cmp_cli_data/test_connection.csv
test/run_tests.pl: Improve indentation parsing workaround for VFO and VFP mode
test/run_tests.pl: Improve newline output for VFO and VFP mode

…thout proxy Fixes openssl#12156

…ODE_UNSAFE_FOR_PRODUCTION

… VFP mode

romen · 2020-06-21T11:19:15Z

@DDvO given you are fixing VFO and VFP, what about documenting them? Do you think it would fit in this PR or is it better left as a separate one?

At the moment we have this:

openssl/INSTALL.md

Lines 1657 to 1704 in 200ae2e

    
           Test Failures 
        
           ------------- 
        
           If some tests fail, look at the output.  There may be reasons for the failure 
        
           that isn't a problem in OpenSSL itself (like a malfunction with Perl). 
        
           You may want increased verbosity, that can be accomplished like this: 
        
           Verbosity on failure only (make macro VERBOSE_FAILURE or VF): 
        
               $ make VF=1 test                                 # Unix 
        
               $ mms /macro=(VF=1) test                         ! OpenVMS 
        
               $ nmake VF=1 test                                # Windows 
        
           Full verbosity (make macro VERBOSE or V): 
        
               $ make V=1 test                                  # Unix 
        
               $ mms /macro=(V=1) test                          ! OpenVMS 
        
               $ nmake V=1 test                                 # Windows 
        
           If you want to run just one or a few specific tests, you can use 
        
           the make variable TESTS to specify them, like this: 
        
               $ make TESTS='test_rsa test_dsa' test            # Unix 
        
               $ mms/macro="TESTS=test_rsa test_dsa" test       ! OpenVMS 
        
               $ nmake TESTS='test_rsa test_dsa' test           # Windows 
        
           And of course, you can combine (Unix example shown): 
        
               $ make VF=1 TESTS='test_rsa test_dsa' test 
        
           You can find the list of available tests like this: 
        
               $ make list-tests                                # Unix 
        
               $ mms list-tests                                 ! OpenVMS 
        
               $ nmake list-tests                               # Windows 
        
           Have a look at the manual for the perl module Test::Harness to 
        
           see what other HARNESS_* variables there are. 
        
           If you find a problem with OpenSSL itself, try removing any 
        
           compiler optimization flags from the CFLAGS line in Makefile and 
        
           run "make clean; make" or corresponding. 
        
           To report a bug please open an issue on GitHub, at 
        
           <https://github.com/openssl/openssl/issues>. 
        
           For more details on how the make variables TESTS can be used, 
        
           see section [Running Selected Tests](#running-selected-tests) below.

I don't know what would be best: expanding that section or having a new markdown file inside test/ describing in detail the various verbosity options while adding just a reference to it in INSTALL.md?

What do you think?

romen

LGTM.

I left a note about documenting the VFO/VFP options, but I will leave to @DDvO to decide if it should be done in a separate PR.

romen

LGTM, thanks @DDvO !

DDvO · 2020-06-21T13:56:15Z

Thanks @romen for reviewing this and your comments:

@DDvO given you are fixing VFO and VFP, what about documenting them?

Very good point!

Do you think it would fit in this PR or is it better left as a separate one?

I don't know what would be best: expanding that section or having a new markdown file inside test/ describing in detail the various verbosity options while adding just a reference to it in INSTALL.md?

What do you think?

I'd say it makes most sense to do a quick documentation extension as part of this PR
(I've just added a respective commit).

After this one has been merged I'll do a further PR for cleaning up INSTALL.md, by moving test-related documentation to test/README.md and referencing it from INSTALL.md (there are at least two sections in that file where such a reference would be helpful in my view).

openssl-machine · 2020-06-22T12:00:12Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

DDvO · 2020-06-22T14:43:40Z

Merged - thanks @kroeckx and @romen!

Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

…thout proxy Fixes #12156 Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

…ODE_UNSAFE_FOR_PRODUCTION Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

… VFP mode Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

levitte · 2020-06-24T09:43:51Z

INSTALL.md

-    $ ./config
+    ./config
    Operating system: x86-whatever-minix
    This system (minix) is not supported. See file INSTALL for details.


Damnit, I didn't pay enough attention.

A reason for the $ was to clearly mark what is prompted commands and what is command output. It's even explained here (which, interestingly enough, you ignored in all kinds of ways):

openssl/INSTALL.md

Lines 73 to 81 in 922f156

Commands

--------

Any line starting with a dollar sign is a command line.

$ command

The dollar sign indicates the shell prompt and is not to be entered as

part of the command.

I find it incredibly irritating that things like this get "slipped in" in PRs that have nothing to do with it.

Indeed I overlooked that explanation, but I did suspect that the $ was used for that purpose.

Still I don't like it, because in particular if there is a number of shell commands in a row, such as in

$ mkdir /var/tmp/openssl-build $ cd /var/tmp/openssl-build $ whatever

you cannot simply copy&paste that series of commands, while with

mkdir /var/tmp/openssl-build cd /var/tmp/openssl-build whatever

you can.

And I suppose there are just a few cases where command input and output could be confused.
In such cases we could, e.g., insert a blank line to help understanding this distinction.

Regardless, if you want to change that, having that "slip in" in a PR that's about something different is not a good way to do it.

I find it incredibly irritating that things like this get "slipped in" in PRs that have nothing to do with it.

Yeah, I should better have done this in a separate PR for clarity.
It just had come up along with the review of this PR, so for simplicity I added a commit for that here.

Regardless, if you want to change that, having that "slip in" in a PR that's about something different is not a good way to do it.

Sorry for that.

I've just checked: in this (merged) PR this is the only case where command input and output got mixed.
Here I suggest simply dropping the line

./config

because it is already mentioned in the sentence above it.

@levitte, are you okay if I do this nit fix along with #12109 or #12232,
or should I also in this case do a separate PR for it?

And of course we should be consistent about using $ or not,
and if there is consensus to drop it also remove the remark in INSTALL.md on its purpose quoted above.

BTW, the $ symbols is not commonly used under Windows, where typically a > is printed as prompt.

I just realized that already today at noon (CEST) a PR restoring the $ has been done: #12257.

DDvO mentioned this pull request Jun 17, 2020

Chunk 12 of CMP contribution to OpenSSL: CLI-based test #11998

Closed

2 tasks

mouse07410 mentioned this pull request Jun 17, 2020

Extended tests are failing cmp tests #12145

Closed

DDvO mentioned this pull request Jun 18, 2020

Master fails "81-test_cmp.cli.t" #12156

Closed

DDvO force-pushed the fix_cmp_cli_tests branch from 2684529 to e7c5cd4 Compare June 18, 2020 05:31

DDvO changed the title ~~WIP: Fix CMP CLI tests~~ Fix CMP CLI tests Jun 18, 2020

DDvO requested a review from kroeckx June 19, 2020 18:32

kroeckx reviewed Jun 20, 2020

View reviewed changes

crypto/cmp/cmp_ctx.c Outdated Show resolved Hide resolved

kroeckx reviewed Jun 20, 2020

View reviewed changes

doc/man3/OSSL_CRMF_MSG_set0_validity.pod Outdated Show resolved Hide resolved

kroeckx reviewed Jun 20, 2020

View reviewed changes

doc/man3/OSSL_CRMF_MSG_set0_validity.pod Outdated Show resolved Hide resolved

DDvO force-pushed the fix_cmp_cli_tests branch from 98c210e to 2ae3512 Compare June 20, 2020 15:25

DDvO added 5 commits June 20, 2020 17:35

Fix CMP -days option range checking and test failing with enable-ubsan

934257a

81-test_cmp_cli.t: Do connections to 127.0.0.1 (e.g., Mock server) wi…

843e5f7

…thout proxy Fixes openssl#12156

Disable tests in cmp_vfy_test.c that make no sense if FUZZING_BUILD_M…

1878415

…ODE_UNSAFE_FOR_PRODUCTION

81-test_cmp_cli.t: Disable CLI-based tests in case fuzzing is enabled

de1e789

test/run_tests.pl: Improve newline output for VFO and VFP mode

5062b3a

DDvO force-pushed the fix_cmp_cli_tests branch from 2ae3512 to ad506f6 Compare June 20, 2020 15:36

DDvO added 2 commits June 20, 2020 17:45

test/run_tests.pl: Improve indentation parsing workaround for VFO and…

5c4d441

… VFP mode

Speed-up for tests in 81-test_cmp_cli_data/test_connection.csv

db15402

DDvO force-pushed the fix_cmp_cli_tests branch from ad506f6 to db15402 Compare June 20, 2020 15:46

romen added the branch: master Applies to master branch label Jun 21, 2020

romen approved these changes Jun 21, 2020

View reviewed changes

romen added the approval: done This pull request has the required number of approvals label Jun 21, 2020

test/run_tests.pl: Document new VFO and VFP modes in INSTALL.md

1ae5c43

romen approved these changes Jun 21, 2020

View reviewed changes

DDvO added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Jun 22, 2020

DDvO closed this Jun 22, 2020

openssl-machine pushed a commit that referenced this pull request Jun 22, 2020

Fix CMP -days option range checking and test failing with enable-ubsan

11baa47

Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

openssl-machine pushed a commit that referenced this pull request Jun 22, 2020

81-test_cmp_cli.t: Do connections to 127.0.0.1 (e.g., Mock server) wi…

7d40fac

…thout proxy Fixes #12156 Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

openssl-machine pushed a commit that referenced this pull request Jun 22, 2020

81-test_cmp_cli.t: Disable CLI-based tests in case fuzzing is enabled

713b3f7

Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

openssl-machine pushed a commit that referenced this pull request Jun 22, 2020

Disable tests in cmp_vfy_test.c that make no sense if FUZZING_BUILD_M…

be7f84e

…ODE_UNSAFE_FOR_PRODUCTION Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

openssl-machine pushed a commit that referenced this pull request Jun 22, 2020

Speed-up for tests in 81-test_cmp_cli_data/test_connection.csv

1e24c82

Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

openssl-machine pushed a commit that referenced this pull request Jun 22, 2020

test/run_tests.pl: Improve newline output for VFO and VFP mode

6bb74ec

Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

openssl-machine pushed a commit that referenced this pull request Jun 22, 2020

test/run_tests.pl: Improve indentation parsing workaround for VFO and…

93a7d24

… VFP mode Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

openssl-machine pushed a commit that referenced this pull request Jun 22, 2020

test/run_tests.pl: Document new VFO and VFP modes in INSTALL.md

b0d5c1c

Reviewed-by: Nicola Tuveri <[email protected]> (Merged from #12175)

DDvO mentioned this pull request Jun 22, 2020

Move test-related detail from INSTALL.md to new test/README.md #12232

Closed

levitte reviewed Jun 24, 2020

View reviewed changes

This was referenced Jun 24, 2020

Master fails 20-test_app.t #12246

Closed

Fix some CMP related msan failures #12275

Closed

	Commands
	--------

	Any line starting with a dollar sign is a command line.

	$ command

	The dollar sign indicates the shell prompt and is not to be entered as
	part of the command.

Uh oh!

Conversation

DDvO commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slontis commented Jun 17, 2020

Uh oh!

DDvO commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented Jun 18, 2020

Uh oh!

DDvO commented Jun 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DDvO commented Jun 20, 2020

Uh oh!

romen commented Jun 21, 2020

Uh oh!

romen left a comment

Choose a reason for hiding this comment

Uh oh!

romen left a comment

Choose a reason for hiding this comment

Uh oh!

DDvO commented Jun 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openssl-machine commented Jun 22, 2020

Uh oh!

DDvO commented Jun 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DDvO Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DDvO commented Jun 17, 2020 •

edited

Loading

DDvO commented Jun 18, 2020 •

edited

Loading

DDvO commented Jun 21, 2020 •

edited

Loading

DDvO Jun 24, 2020 •

edited

Loading