Evp parser tweak #3506

richsalz · 2017-05-20T23:17:47Z

This is the evp_test part of #3471. It moves the parsing to be more similar to bn_test. I hope my text PR in this area will refactor out common code.

dot-asm · 2017-05-22T09:20:51Z

test/evpdigest.txt

@@ -90,7 +91,8 @@ Digest = BLAKE2b512
 Input = 000102030405060708090A0B0C0D0E0F101112131415161718191A1B1C1D1E1F202122232425262728292A2B2C2D2E2F303132333435363738393A3B3C3D3E3F404142434445464748494A4B4C4D4E4F505152535455565758595A5B5C5D5E5F606162636465666768696A6B6C6D6E6F707172737475767778797A7B7C7D7E7F8081
 Output = DF0A9D0C212843A6A934E3902B2DD30D17FBA5F969D2030B12A546D8A6A45E80CF5635F071F0452E9C919275DA99BED51EB1173C1AF0518726B75B0EC3BAE2B5

-# SHA tests from RFC6234 section 8.5
+Title = SHA(1) tests (from shatest.c)


shatest.c is not present anymore, no point to mention it. I'd suggest just "SHA1 tests" here and either "SHA2 tests" prior SHA224/256/384/512 or separate SHAxxx titles...

I just made it "SHA tests"

dot-asm · 2017-05-22T09:23:11Z

test/evppbe.txt

 Result = SCRYPT_ERROR
+#maxmem = 10000000000


What does move of #maxmem mean? That parser wouldn't tolerate it in old position?

yes. that will be fixed in a future PR.

dot-asm · 2017-05-22T09:28:30Z

test/evppkey.txt

 # TEST CURVE secp112r1
+Title = ECDH Tests: test with randomly generated keys for all the listed curves


This is kind broken. In this case Title refers to multiple curves, secp112r1 being just one, and flipping order kind of suggests that there is one curve and we exercise different algorithms on it.

richsalz · 2017-05-22T12:30:14Z

additional commit pushed

dot-asm · 2017-05-22T12:39:57Z

test/evpdigest.txt

@@ -90,7 +91,8 @@ Digest = BLAKE2b512
 Input = 000102030405060708090A0B0C0D0E0F101112131415161718191A1B1C1D1E1F202122232425262728292A2B2C2D2E2F303132333435363738393A3B3C3D3E3F404142434445464748494A4B4C4D4E4F505152535455565758595A5B5C5D5E5F606162636465666768696A6B6C6D6E6F707172737475767778797A7B7C7D7E7F8081
 Output = DF0A9D0C212843A6A934E3902B2DD30D17FBA5F969D2030B12A546D8A6A45E80CF5635F071F0452E9C919275DA99BED51EB1173C1AF0518726B75B0EC3BAE2B5

-# SHA tests from RFC6234 section 8.5
+Title = SHA tests


I'd say it's inappropriate to drop the reference to doc/spec.

If anything there should be more references, not less...

That makes sense; restored comment; updated commit pushed.

dot-asm · 2017-05-22T12:47:13Z

test/evppkey.txt

-
-
-# TEST CURVE secp112r1
+Title = TEST CURVE secp112r1


But this implies that you would have to entitle changes from one curve to next. Suggestion was rather to entitle it with "EDCH tests" and then have comments delimiting different curves...

richsalz · 2017-05-22T12:55:35Z

I misunderstood. Updated commit pushed.

dot-asm · 2017-05-22T15:49:26Z

test/evppkey.txt

@@ -693,7 +695,7 @@ Result = KEYOP_INIT_ERROR
 Function = pkey_pss_init
 Reason = invalid salt length

-# ECDH tests
+Title ECDH tests


[When red-crossing] CI says

# ERROR: # Line 364993016: Missing '=' in test file

I assume it's talking about this line. But error message calls even for question about line number!

error message calls even for question about line number!

I naturally mean 364993016, which is not helpful.

right. that's a puzzle. looking at it.

i think i fixed it. updated commit pushed.

I see no extra commits...

doh! pushed.

dot-asm · 2017-05-23T09:22:35Z

Line numbering is still broken. If I corrupt first Title in evpciph.txt it complains about line number -1185918280. But line numbers for corrupted test data are correct.

dot-asm · 2017-05-23T12:50:22Z

While you are at it and since you refer to bn_test. Can you look at line numbering in failed tests in bn_test? For example if you corrupt first test, it gets attributed to line 0. If you corrupt other than first, it gets attributed to line past previous test. While they should be attributed to first significant line of current test [like evp_test].

richsalz · 2017-05-23T12:53:10Z

I don't see the Title line number bug:

1..1
    # Subtest: run_file_tests
    1..1
    # INFO:  @ test/evp_test.c:2521
    # Reading test/evpciph.txt

    # ERROR:  @ test/evp_test.c:2297
    # Line 22: Missing '=' in test file

    # INFO:  @ test/evp_test.c:2536
    # Completed 0 tests with 1 errors and 0 skipped

    not ok 1 - test/evpciph.txt
not ok 1 - run_file_tests
        exit 1

I'll look at bn_test. My plan is for the next PR to merge the parsing routines...

dot-asm · 2017-05-23T12:54:00Z

Speaking of Titles. There are too little of them. For example if you run evpciph.txt it ~~test starting~~ reports "DES EDE3 CFB1", "AES (from FIPS-197 test vectors)" and "AES tests from NIST document SP800-38A". But past last one there are loads of non-AES tests, then followed by AES-GCM, AES-OCB, AES-XTS, and again non-AES tests. Bottom line is that output from evp_test is effectively misleading.

dot-asm · 2017-05-23T13:00:08Z

   # ERROR:  @ test/evp_test.c:2297
   # Line 22: Missing '=' in test file

What can I say? I get different number every time I run, e.g. 1751751624, -2011814616, -525217528, 1936404088... What's your top? Mine is 135cffc27b9adb6df2a0d071e7f5168ddd59b73a

richsalz · 2017-05-23T13:03:16Z

#3528 to fix the bntest issue you found.
My top is

commit f672095875ab6c5a0bfe03586a15020422234a66
Author: Rich Salz <rsalz@openssl.org>
Date:   Mon May 22 08:29:44 2017 -0400

    Fix Title comments (via Andy)

And I just now did a push -f.

dot-asm · 2017-05-23T13:11:42Z

I've got f672095875ab6c5a0bfe03586a15020422234a66 now, but it still prints random line numbers in reply to Title without =...

richsalz · 2017-05-23T13:14:19Z

I'm using clang with debug flags. I'll try GCC.
Also pushed new commit to add more titles to evpciph.txt

richsalz · 2017-05-23T13:15:46Z

ah, gcc shows the error. dang.

richsalz · 2017-05-23T13:23:35Z

well this is strange:

(gdb) b 2297
Breakpoint 1 at 0x40910d: file test/evp_test.c, line 2297.
(gdb) r test/evpciph.txt
Starting program: /home/rsalz/openssl/github/test/evp_test test/evpciph.txt
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
1..1
    # Subtest: run_file_tests
    1..1
    # INFO:
    # Reading test/evpciph.txt


Breakpoint 1, split_line (t=0x7fffffff9ad0, keyword=0x7fffffff9a78,
    value=0x7fffffff9a80) at test/evp_test.c:2297
2297            TEST_error("Line %d: Missing '=' in test file", t->line);
(gdb) p t->line
$1 = 22
(gdb) n
    # ERROR:
    # Line -26296: Missing '=' in test file

I believe the problem is that test_error_c90 calls va_start/va_end`` and then calls test_fail_message``` which also does. Those nested calls break. This diff seems to fix it:

diff --git a/test/testutil/tests.c b/test/testutil/tests.c
index 3f75691..a5538e7 100644
--- a/test/testutil/tests.c
+++ b/test/testutil/tests.c
@@ -524,7 +524,7 @@ void test_error_c90(const char *desc, ...)
     va_list ap;

     va_start(ap, desc);
-    test_fail_message(NULL, NULL, -1, NULL, NULL, NULL, NULL, desc, ap);
+    test_fail_message_va(NULL, NULL, -1, NULL, NULL, NULL, NULL, desc, ap);
     va_end(ap);
 }

richsalz · 2017-05-23T13:24:38Z

pushed a new commit with that fix.

dot-asm · 2017-05-23T13:29:37Z

add more titles to evpciph.txt

What about others? E.g. evpdigest.txt says ISO/IEC 10118-3 and does Whirlpool test. Yes, Whirlpool is there, but so are SHA and RIPEMD-160... evpmac.txt has only SIPHASH Title, but more algorithms than that...

richsalz · 2017-05-23T13:30:42Z

I promise to do this in a separate PR. Will start shortl. Don't want to distract from this, which is all asbout the C code.

dot-asm · 2017-05-23T13:35:42Z

I promise to do this in a separate PR

Fair enough. Then it makes perfect sense to omit evpciph.txt commit here :-)

dot-asm · 2017-05-23T13:47:35Z

test_error_c90 calls va_start/va_end ... fix pushed

Confirmed.

richsalz · 2017-05-23T14:04:24Z

#3529 has the additional titles stuff.
So still waiting on approval for this :)

richsalz · 2017-05-23T15:06:59Z

Moved all title changes to #3529

dot-asm · 2017-05-23T15:25:14Z

I expect this to be re-based after #3529 is merged, and pass through travis, only then I'll look into it...

Re-order and comment on the functions

richsalz · 2017-05-23T15:35:16Z

rebased an squashed and pushed. waiting for travis.

dot-asm · 2017-05-23T17:58:24Z

test/evp_test.c

-    }
-    return 1;
-}
-
 static int compare_mem(unsigned char *expected, size_t expected_len,
                       unsigned char *got, size_t  got_len)
 {
    if (!TEST_mem_eq(expected, expected_len, got, got_len))
        return 0;
    return 1;
 }


On side note one can probably argue that it calls to compare_mem should be replaced with TEST_mem_eq with rationale that __LINE__ would be more meaningful if it was compare_mem caller's.

So that was done because the diff output wasn't totally useful before and having the local variables named expected and got was more helpful. Removing it is trivial, should I?

Note "probably" in the side note. It means it's just an idea that might be worth considering. It might be sensible or it might not. But I can't prevent you from testing and telling how it looks. For example with regard to expected appearing in printout. It might be important, but then it might be possible to solve it by renaming some structure fields... Once again, something to consider...

Yeah, I know. But you're the one who has raised this issue before. So tell me which you prefer:

# ERROR: (memory) 'mdata->output == mac' failed @ test/evp_test.c:1381 # --- mdata->output # +++ mac # 0000:-b3817f04ba25a8e6 6df67214c7550293 # 0000:+a3817f04ba25a8e6 6df67214c7550293 # ^^

Or this

# ERROR: (memory) 'expected == got' failed @ test/evp_test.c:328 # --- expected # +++ got # 0000:-b3817f04ba25a8e6 6df67214c7550293 # 0000:+a3817f04ba25a8e6 6df67214c7550293 # ^^

I for one do care for line number in first output. And it's possible to make output more explicit by renaming output field to expected, so that it would read mdata->expected. However! Since going through this request was hell (coloured "Files changed" is impossible to follow), I'd insist on merging it as it is now and do further improvements in separate request. I can't handle going trough it again...

:) It's not like you're review @levitte's STORE stuff :) Merged this. New PR coming to rename the fields. Thanks for the careful review and feedback!

Re-order and comment on the functions Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #3506)

Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #3506)

richsalz · 2017-05-24T15:39:26Z

See #3539

dot-asm reviewed May 22, 2017

View reviewed changes

richsalz mentioned this pull request May 23, 2017

Fix line numbering for errors. #3528

Closed

richsalz mentioned this pull request May 23, 2017

Add titles to groups of EVP tests #3529

Closed

richsalz self-assigned this May 23, 2017

Revise evp_test parser; make like bn_test

97fe9b4

Re-order and comment on the functions

Fix va_arg all in test_error_c90

e819095

dot-asm reviewed May 23, 2017

View reviewed changes

dot-asm approved these changes May 23, 2017

View reviewed changes

dot-asm added branch: master Merge to master branch approval: done This pull request has the required number of approvals labels May 23, 2017

levitte pushed a commit that referenced this pull request May 24, 2017

Revise evp_test parser; make like bn_test

c49e0b0

Re-order and comment on the functions Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #3506)

levitte pushed a commit that referenced this pull request May 24, 2017

Fix va_arg all in test_error_c90

dd59187

Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #3506)

richsalz closed this May 24, 2017

richsalz deleted the evp-parser-tweak branch May 24, 2017 12:43

richsalz mentioned this pull request May 24, 2017

Label test structs with "wanted" #3539

Closed

romen mentioned this pull request Sep 7, 2018

[1.1.0] Narrow evptests gap between 1.1.1 and 1.1.0 #7152

Closed

1 task

		# TEST CURVE secp112r1
		Title = ECDH Tests: test with randomly generated keys for all the listed curves

Evp parser tweak #3506

Evp parser tweak #3506

Conversation

richsalz commented May 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richsalz commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richsalz commented May 22, 2017

dot-asm May 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm commented May 23, 2017

dot-asm commented May 23, 2017

richsalz commented May 23, 2017

dot-asm commented May 23, 2017 • edited

dot-asm commented May 23, 2017

richsalz commented May 23, 2017

dot-asm commented May 23, 2017

richsalz commented May 23, 2017

richsalz commented May 23, 2017

richsalz commented May 23, 2017

richsalz commented May 23, 2017

dot-asm commented May 23, 2017

richsalz commented May 23, 2017

dot-asm commented May 23, 2017

dot-asm commented May 23, 2017

richsalz commented May 23, 2017

richsalz commented May 23, 2017

dot-asm commented May 23, 2017

richsalz commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richsalz commented May 24, 2017

dot-asm May 22, 2017 •

edited

dot-asm commented May 23, 2017 •

edited