Improve fork helper robustness #297

2026-04-03T11:09:19-05:00

gilles-peskine-arm commented

2026-04-03 11:09:19 -05:00

(Migrated from github.com)

Resolves most of https://github.com/Mbed-TLS/mbedtls-framework/issues/295, except for the metatests which are added in a consuming branch instead: https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/737.

PR checklist

TF-PSA-Crypto development PR https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/737
TF-PSA-Crypto 1.1 PR not required because: can be updated whenever
mbedtls development PR not required because: does not use fork helper
mbedtls 4.1 PR not required because: does not use fork helper
mbedtls 3.6 PR not required because: can be updated whenever

Resolves most of https://github.com/Mbed-TLS/mbedtls-framework/issues/295, except for the metatests which are added in a consuming branch instead: https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/737. ## PR checklist - [x] **TF-PSA-Crypto development PR** https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/737 - [x] **TF-PSA-Crypto 1.1 PR** not required because: can be updated whenever - [x] **mbedtls development PR** not required because: does not use fork helper - [x] **mbedtls 4.1 PR** not required because: does not use fork helper - [x] **mbedtls 3.6 PR** not required because: can be updated whenever

bjwtaylor (Migrated from github.com) reviewed

2026-04-03 11:09:19 -05:00

bensze01 (Migrated from github.com) reviewed

2026-04-03 11:09:19 -05:00

bjwtaylor (Migrated from github.com) reviewed

2026-04-08 07:35:45 -05:00

tests/src/fork_helpers.c

					
				@ -155,90 +156,93 @@ int mbedtls_test_fork_run_child(

				{

				    *child_output_length = 0;

Would any scenario where less than child_test_info not be a failure here? I think it's minor, however presumably if the file was corrupted or truncated we would want it to fail here?

tests/src/fork_helpers.c

I suspect this has probably been consider, but just in case. This is a bit of a regression on the previous implementation as it now requires create/unlink permission, whereas before it could run in read only. Not sure it this will effect anyone though? Maybe if not already considered it might be worth using mkstemp()/tmpfile() as it creates a less predictable filename?

gilles-peskine-arm (Migrated from github.com) reviewed

2026-04-08 08:20:16 -05:00

tests/src/fork_helpers.c

We already use files in the current directory for tests that use persistent keys. So yes, it's an additional requirement on the runtime environment compared with the previous fork helper code, but it isn't an additional requirement compared for our crypto testing as a whole.

I deliberately don't use tmpfile() or similar to avoid concerns about choosing the right temporary directory, having to worry that it might require unpredictable file names, etc.

We already use files in the current directory for tests that use persistent keys. So yes, it's an additional requirement on the runtime environment compared with the previous fork helper code, but it isn't an additional requirement compared for our crypto testing as a whole. I deliberately don't use `tmpfile()` or similar to avoid concerns about choosing the right temporary directory, having to worry that it might require unpredictable file names, etc.

gilles-peskine-arm (Migrated from github.com) reviewed

2026-04-08 08:25:44 -05:00

tests/src/fork_helpers.c

					
				@ -155,90 +156,93 @@ int mbedtls_test_fork_run_child(

				{

				    *child_output_length = 0;

Good point, I meant to read one record of the intended size, but I wrote it the other way round out of habit. I'll fix that.

I didn't bother writing a metatest for a truncated file because it's an unlikely failure that would require us to go out of our way. We don't normally test test code to that extent.

Good point, I meant to read one record of the intended size, but I wrote it the other way round out of habit. I'll fix that. I didn't bother writing a metatest for a truncated file because it's an unlikely failure that would require us to go out of our way. We don't normally test test code to that extent.

bjwtaylor (Migrated from github.com) reviewed

2026-04-09 02:13:41 -05:00

tests/src/fork_helpers.c

Ok, sounds reasonable.

bjwtaylor (Migrated from github.com) reviewed

2026-04-09 02:15:19 -05:00

bensze01 (Migrated from github.com) requested changes

2026-08-12 09:43:02 -05:00

bensze01 (Migrated from github.com) left a comment

A few suggestions and typos

tests/include/test/helpers.h

Improve wording

 * \return          \c 0 if the test case was recorded as a failure, otherwise \c 1.

Improve wording ```suggestion * \return \c 0 if the test case was recorded as a failure, otherwise \c 1. ```

tests/include/test/helpers.h

Clarify the behaviour of the function

 * \brief           Record the current test case as a failure if
 *                  parameter needle is not a substring of parameter haystack.

Clarify the behaviour of the function ```suggestion * \brief Record the current test case as a failure if * parameter needle is not a substring of parameter haystack. ```

tests/include/test/macros.h

					
				@ -121,6 +121,25 @@

				        goto exit;                                                  \

I think this is easier to understand with the parameter names from mbedtls_test_strstr().

 * \param haystack     A string-valued expression to evaluate.
 * \param needle     Another string-valued expression to evaluate.
 */
#define TEST_STRSTR(haystack, needle)                                       \
    do {                                                                \
        if (!mbedtls_test_strstr("strstr(" #haystack ", " #needle ")",      \
                                 __LINE__, __FILE__,                    \
                                 expr1, expr2)) {                       \
            goto exit;                                                  \
        }                                                               \
    } while (0)

I think this is easier to understand with the parameter names from `mbedtls_test_strstr()`. ```suggestion * \param haystack A string-valued expression to evaluate. * \param needle Another string-valued expression to evaluate. */ #define TEST_STRSTR(haystack, needle) \ do { \ if (!mbedtls_test_strstr("strstr(" #haystack ", " #needle ")", \ __LINE__, __FILE__, \ expr1, expr2)) { \ goto exit; \ } \ } while (0) ```

tests/src/fork_helpers.c

					
				@ -25,20 +26,20 @@

				/** Child exit code for mbedtls_test_fork_run_child().

Typo

     * - on pass, the output (up to the end of the file).

Typo ```suggestion * - on pass, the output (up to the end of the file). ```

tests/src/fork_helpers.c

					
				@ -67,7 +68,7 @@ static int probably_running_under_valgrind(void)

				__attribute__((__noreturn__))

Nit: I don't think this is necessary if the implementation of fwrite complies with POSIX.
TEST_ASSERT_ERRNO won't read the (stale) value of errno if n == length since you've switched the order of 1 and length

Nit: I don't think this is necessary if the implementation of `fwrite` complies with POSIX. `TEST_ASSERT_ERRNO` won't read the (stale) value of `errno` if `n == length` since you've switched the order of `1` and `length`

Nit: && length != 0 is unnecessary, since you've switched the positions of the arguments 1 and length.

Nit: ` && length != 0` is unnecessary, since you've switched the positions of the arguments `1` and `length`.

tests/src/fork_helpers.c

The output produced by %p is implementation defined, so I'm unsure if the size calculation for it is always correct.

How about using mkstemp() with the template "mbedtls_test_fork_run_child.tmp-XXXXXX"? We can also add the pid to the template if you'd like - but I'm not sure it's necessary.

The output produced by `%p` is implementation defined, so I'm unsure if the size calculation for it is always correct. How about using `mkstemp()` with the template "mbedtls_test_fork_run_child.tmp-XXXXXX"? We can also add the pid to the template if you'd like - but I'm not sure it's necessary.

gilles-peskine-arm (Migrated from github.com) reviewed

2026-08-12 10:13:33 -05:00

tests/src/fork_helpers.c

					
				@ -67,7 +68,7 @@ static int probably_running_under_valgrind(void)

				__attribute__((__noreturn__))

POSIX only states that “if a write error occurs, (…) errno shall be set to indicate the error.” It's a classic gotcha that historical implementations don't always set errno = 0 on success: they may leave it unchanged, or even leave a nonzero result from an internal call that failed in a non-fatal way. Modern implementations usually do set errno = 0 on success, but I'm old enough to be in the habit of setting it if I care.

[POSIX](https://pubs.opengroup.org/onlinepubs/9799919799/functions/fwrite.html#tag_17_199_03) only states that “if a write error occurs, (…) errno shall be set to indicate the error.” It's a classic gotcha that historical implementations don't always set `errno = 0` on success: they may leave it unchanged, or even leave a nonzero result from an internal call that failed in a non-fatal way. Modern implementations usually do set `errno = 0` on success, but I'm old enough to be in the habit of setting it if I care.

gilles-peskine-arm (Migrated from github.com) reviewed

2026-08-12 10:18:19 -05:00

tests/src/fork_helpers.c

I'd rather not depend on mkstemp, which might not exist on embedded platforms where we might want to test fork().

The output of %p is unspecified, but I think hex for non-null pointers is really common. But I should check the return value of snprintf.

I'd rather not depend on `mkstemp`, which might not exist on embedded platforms where we might want to test `fork()`. The output of `%p` is unspecified, but I think hex for non-null pointers is really common. But I should check the return value of `snprintf`.

bensze01 (Migrated from github.com) reviewed

2026-08-12 10:18:48 -05:00

tests/src/fork_helpers.c

					
				@ -67,7 +68,7 @@ static int probably_running_under_valgrind(void)

				__attribute__((__noreturn__))

Sure, I was just pointing out that we don't read the undefined value of errno in the success case anyways.

I got the impression that you fixed the issue of fwrite() returning 0 if size == 0 one way (requiring that you overwrite errno), then switched the size and count parameters around so now n == length is always true and we no longer need to reset errno (like we don't anywhere else in this file).

Sure, I was just pointing out that we don't read the undefined value of `errno` in the success case anyways. I got the impression that you fixed the issue of `fwrite()` returning `0` if `size == 0` one way (requiring that you overwrite `errno`), then switched the `size` and `count` parameters around so now `n == length` is always true and we no longer need to reset `errno` (like we don't anywhere else in this file).

gilles-peskine-arm (Migrated from github.com) reviewed

2026-08-12 10:23:22 -05:00

tests/include/test/helpers.h

I don't see how this is more informative, since the readers doesn't know the parameters yet when they read the brief description.

gilles-peskine-arm commented

2026-08-12 14:53:22 -05:00

(Migrated from github.com)

There are a lot of CI failures, probably because this branch is too old. I'm going to rebase on top of the head of main. The last reviewed commit is f61edfd639 → dcf3f2b248

There are a lot of CI failures, probably because this branch is too old. I'm going to rebase on top of the head of `main`. The last reviewed commit is f61edfd639390fea15756361d83466078ee000e8 → dcf3f2b248b8bd07321e52c8c6505994958ac17a

bjwtaylor (Migrated from github.com) reviewed

2026-09-22 09:45:26 -05:00

tests/src/fork_helpers.c

					
				@ -155,90 +156,93 @@ int mbedtls_test_fork_run_child(

				{

Does this account for truncation similar to this https://github.com/Mbed-TLS/mbedtls-framework/pull/297#discussion_r3766902665

Should the return be checked?

tests/src/fork_helpers.c

Does this also need to check the ferror?

gilles-peskine-arm commented

2026-09-22 13:43:22 -05:00

(Migrated from github.com)

The CI is failing on TF-PSA-Crypto development. I need to rebase this branch again to get a meaningful CI run. The last commit that Ben reviewed is fd4cef7ad2 → 49e3acd53b

The CI is failing on TF-PSA-Crypto development. I need to rebase this branch again to get a meaningful CI run. The last commit that Ben reviewed is fd4cef7ad21033282b79476f7db526cacb5b5bf2 → 49e3acd53b4700ef50db65f5fc67733c1f130ac7

This pull request is broken due to missing fork information.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'gilles-peskine-arm/fork-helper-unit-tests-create-framework:gilles-peskine-arm/fork-helper-unit-tests-create-framework'

git switch 'gilles-peskine-arm/fork-helper-unit-tests-create-framework'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'main'

git merge --no-ff 'gilles-peskine-arm/fork-helper-unit-tests-create-framework'

git switch 'gilles-peskine-arm/fork-helper-unit-tests-create-framework'

git rebase 'main'

git switch 'main'

git merge --ff-only 'gilles-peskine-arm/fork-helper-unit-tests-create-framework'

git switch 'gilles-peskine-arm/fork-helper-unit-tests-create-framework'

git rebase 'main'

git switch 'main'

git merge --no-ff 'gilles-peskine-arm/fork-helper-unit-tests-create-framework'

git switch 'main'

git merge --squash 'gilles-peskine-arm/fork-helper-unit-tests-create-framework'

git switch 'main'

git merge --ff-only 'gilles-peskine-arm/fork-helper-unit-tests-create-framework'

git switch 'main'

git merge 'gilles-peskine-arm/fork-helper-unit-tests-create-framework'

git push origin 'main'

Sign in to join this conversation.

No reviewers

bjwtaylor

bensze01

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Mbed-TLS/mbedtls-framework!297

				`@ -25,20 +26,20 @@`
				`/** Child exit code for mbedtls_test_fork_run_child().`

				`@ -67,7 +68,7 @@ static int probably_running_under_valgrind(void)`
				`__attribute__((__noreturn__))`

Rows
Columns