Fix bug in redirection of unit test outputs

[gufe44]

Otherwise it builds and tests fine on OpenBSD, builds fine on NetBSD but (edit: fixed in #3540) gets stuck in mbedtls_mpi_gen_prime (corner case limb size) of test_suite_mpi (both stable/x86_64).

[danh-arm]

The test code does not build with this PR. From the Travis logs:

******************************************************************

* test_default_out_of_box: test: main suites make, default config (out-of-box) 

* Thu Jul 30 07:35:27 UTC 2020

******************************************************************

test_suite_aes.cbc ................................................ Use of uninitialized value $skipped in subtraction (-) at scripts/run-test-suites.pl line 127.

Use of uninitialized value $tests in subtraction (-) at scripts/run-test-suites.pl line 127.

FAIL

test_suite_aes.cfb ................................................ Use of uninitialized value $skipped in subtraction (-) at scripts/run-test-suites.pl line 127.

Use of uninitialized value $tests in subtraction (-) at scripts/run-test-suites.pl line 127.

FAIL
...

[danh-arm]

Also needs:

• A signed-off-by line
• A commit message with rationale as to what the problem is why this fixes it (more than just "my config works with this").
• A change log entry.

[gilles-peskine-arm]

This was previously reported in #2311. There's also a proposed patch in that report, but please note that we cannot legally accept that patch, or a patch derived from it, without an explicit permission grant from its author, so you need to write your patch independently. We also haven't confirmed that this patch works.

I would prefer a different approach. Instead of modifying stdout, change references to stdout in host_test.function to verbose_out and set verbose_out and

FILE *verbose_out = stdout;
#if defined(__unix__) || (defined(__APPLE__) && defined(__MACH__))
if (option_verbose) { ... modify verbose_out ... }
#endif

And close verbose_out before exiting if it isn't stdout, so that tools such as ASan don't complain of a memory leak.

[gufe44]

@gilles-peskine-arm, I'm very sorry, can you please clarify why the changes I made modify the behaviour of the program at all? Wouldn't your proposed solution need the tests themselves also to reference verbose_out?

[gilles-peskine-arm]

The test code never references stdout explicitly, or a function that has stdout hard-coded such as printf. It has to work on embedded targets that don't have stdio. Only host_test.function references stdout (and helpers.function, but that code is specific to hosted targets so it should actually have been in host_test.function).

I haven't looked at your patch too closely. It clearly does modify the behavior of the program since make test isn't working anymore on the CI.

[gufe44]

I must have been blind. Would something like this be portable enough?

[gufe44]

Does this need more work? @danh-arm @gilles-peskine-arm

[hanno-becker]

@gufe44 make test is failing in the full configuration (i.e. after running ./scripts/config.py full). Could you investigate?

[gufe44]

@hanno-arm Isn't this a problem on the development branch? I have only looked so far. I think the tests making use of psa_crypto.c fail to gather entropy with mbedtls_entropy_func. Tests pass if test_suite_entropy has been run before and tests/seedfile was created (or if make test is run twice).

[gilles-peskine-arm]

@gufe44 @hanno-arm Indeed there's a gotcha in our test suites: if MBEDTLS_ENTROPY_NV_SEED is enabled (which is not the case by default but is the case in the full config), the unit tests that call the entropy module need an existing seedfile. test_suite_entropy is an exception because it contains a test case for the creation of a seedfile that runs before any test case that needs a seedfile.

Historically this has mostly worked invisibly because the other test suites that call the entropy module come after entropy in alphabetical order. But if you enable MBEDTLS_USE_PSA_CRYPTO (which config.py full does) then test_suite_cipher calls psa_crypto_init which obtains entropy and therefore requires a seedfile. Since cipher comes before entropy, if you haven't created a seedfile manually, psa_crypto_init() fails.

Another failure reason is when the seedfile is too small. The size of the seedfile is determined by
the hash that the entropy module uses. If you run tests with either MBEDTLS_SHA512_C disabled or MBEDTLS_ENTROPY_FORCE_SHA256 enabled, and then run tests with MBEDTLS_SHA512_C enabled or MBEDTLS_ENTROPY_FORCE_SHA256 disabled, and MBEDTLS_ENTROPY_NV_SEED enabled in both cases, the second test run starts with a seedfile that's too small, and again it will fail if something needs entropy before test_suite_entropy.

Enough people have gotten bitten by this that we should fix it rather than document it. #3575

[hanno-becker]

Thank you for your contribution, the patch looks good to me.

There's a trivial style issue remaining but it doesn't block the PR. Feel free to push a commit or leave things as they are.

Apologies for mistakenly blaming your PR for one of the CI failures. As far as I see, they're known issues in the CI, but I'd be grateful if @mpg or @gilles-peskine-arm could double-check and confirm for the latest run, as they're more familiar with this these days.

[gufe44]

Thank you for your review and for pointing out the remaining style issue. Are there any concerns about portability or backporting to LTS branches?

[hanno-becker]

@gufe44 Have you checked whether the underlying issue is present in 2.7 and 2.16?

[gufe44]

Yes. The relevant functions were already present mostly in the same files.