Improve fork helper robustness by gilles-peskine-arm · Pull Request #297 · Mbed-TLS/mbedtls-framework

gilles-peskine-arm · 2026-04-03T16:09:19Z

Resolves most of #295, except for the metatests which are added in a consuming branch instead: Mbed-TLS/TF-PSA-Crypto#737.

PR checklist

TF-PSA-Crypto development PR Add metatests for the fork test helper as unit tests TF-PSA-Crypto#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

bjwtaylor · 2026-04-08T12:31:25Z

+     */
+    /* Make up a unique name using the current (parent) process ID and
+     * a stack address inside that process. */
+    char filename[sizeof("mbedtls_test_fork_run_child-%ld-%p.tmp") +


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?

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.

Ok, sounds reasonable.

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.

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.

bjwtaylor · 2026-04-08T12:34:31Z

+
+    /* The child exited normally. Obtain the test result from the child. */
+    TEST_ASSERT_ERRNO(fseek(file, 0, SEEK_SET) == 0);
+    TEST_ASSERT_ERRNO(fread(&child_test_info, 1, sizeof(child_test_info), file) > 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?

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.

bensze01

A few suggestions and typos

bensze01 · 2026-08-12T12:04:04Z

+    if (mbedtls_test_get_result() == MBEDTLS_TEST_RESULT_SUCCESS && length != 0) {
+        /* Write the output. This could fail on a full disk. Remember to
+         * flush (otherwise the output would likely be truncated). */
+        errno = 0;


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

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.

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).

bensze01 · 2026-08-12T13:10:47Z

-     *       a #mbedtls_test_result_t structure.
+     * The content of the file has the following format:
+     * - mbedtls_test_info_t structure;
+     * - on pass, the ouptut (up to the end of the file).


Typo

Suggested change

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

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

bensze01 · 2026-08-12T13:38:48Z

+     */
+    /* Make up a unique name using the current (parent) process ID and
+     * a stack address inside that process. */
+    char filename[sizeof("mbedtls_test_fork_run_child-%ld-%p.tmp") +


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.

bensze01 · 2026-08-12T14:06:02Z

-    TEST_LE_U(length, size);

-    /* Label called `exit`: this is where TEST_ASSERT() and friends jump to. */
+    if (mbedtls_test_get_result() == MBEDTLS_TEST_RESULT_SUCCESS && length != 0) {


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

bensze01 · 2026-08-12T14:18:04Z

+ * \brief           Record the current test case as a failure based
+ *                  on a substring search.


Clarify the behaviour of the function

Suggested change

* \brief Record the current test case as a failure based

* on a substring search.

* \brief Record the current test case as a failure if

* parameter needle is not a substring of parameter haystack.

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

bensze01 · 2026-08-12T14:21:21Z

+ *                  Alternatively, this can be a null pointer,
+ *                  which is treated as if it was an empty string.
+ *
+ * \return          \c 1 on success, otherwise \c 0.


Improve wording

Suggested change

* \return \c 1 on success, otherwise \c 0.

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

bensze01 · 2026-08-12T14:26:39Z

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


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

Suggested change

* \param expr1 An string-valued expression to evaluate.

* \param expr2 Another string-valued expression to evaluate.

*/

#define TEST_STRSTR(expr1, expr2) \

do { \

if (!mbedtls_test_strstr("strstr(" #expr1 ", " #expr2 ")", \

__LINE__, __FILE__, \

expr1, expr2)) { \

goto exit; \

} \

} while (0)

* \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)

Assert on strstr(), and print the strings on failure. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Use a temporary file instead of a pipe to communicate the test result and the output from the child to the parent. This makes the data flow and the control flow easier because the code can seek back and forth, whereas a pipe required doing things in order. In particular, there are now fewer special cases to manage, and the parent can read the data in the most convenient order. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

If the child reports an output length that's larger than the buffer, report it rather than overread the buffer. The parent would catch the excess length anyway, but having the child catch it makes the behavior more uniform with respect to the presence or absence of sanitizers. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

In the unlikely event of a corrupted file or I/O error, don't accept a partially written status. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Rather than seek forth and back, read up to the buffer size, and then check that the file wasn't larger than the buffer size. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

In case the `%p` output is larger than expected. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

This block now works even when `length == 0`. Setting `errno = 0` doesn't really help. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm · 2026-08-12T19:53:22Z

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 f61edfd → dcf3f2b

gilles-peskine-arm added this to Roadmap pull requests (new board) Apr 3, 2026

gilles-peskine-arm added needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels Apr 3, 2026

github-project-automation Bot moved this to In Development in Roadmap pull requests (new board) Apr 3, 2026

gilles-peskine-arm force-pushed the fork-helper-unit-tests-create-framework branch 3 times, most recently from bc83f4e to cd6fb18 Compare April 4, 2026 17:55

bjwtaylor self-requested a review April 8, 2026 12:23

bjwtaylor reviewed Apr 8, 2026

View reviewed changes

gilles-peskine-arm requested a review from bjwtaylor April 8, 2026 14:22

gilles-peskine-arm added the needs-review Every commit must be reviewed by at least two team members. label Apr 8, 2026

gilles-peskine-arm requested a review from bensze01 April 8, 2026 14:22

gilles-peskine-arm mentioned this pull request Apr 8, 2026

Rare intermittent failure of DTLS 3d tests Mbed-TLS/mbedtls#8471

Open

bjwtaylor previously approved these changes Apr 9, 2026

View reviewed changes

gilles-peskine-arm mentioned this pull request Apr 9, 2026

Add metatests for the fork test helper as unit tests Mbed-TLS/TF-PSA-Crypto#737

Open

7 tasks

gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Apr 9, 2026

bensze01 requested changes Aug 12, 2026

View reviewed changes

gilles-peskine-arm dismissed bjwtaylor’s stale review via f598590 August 12, 2026 15:34

gilles-peskine-arm requested review from bensze01 and bjwtaylor August 12, 2026 15:34

gilles-peskine-arm added 9 commits August 12, 2026 21:52

New test helper TEST_STRSTR

0b98c3f

Assert on strstr(), and print the strings on failure. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Note about test info requiring string literals in practice

f98fa0e

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

mbedtls_test_fork_run_child: Fix failure when the child outputs nothing

887e8cb

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

mbedtls_test_fork_run_child: Expose file to invasive metatest

53efeab

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Improve robustness against read errors from the temporary file

c0e2f6d

In the unlikely event of a corrupted file or I/O error, don't accept a partially written status. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Slightly simplify reading the child output

dcf3f2b

Rather than seek forth and back, read up to the buffer size, and then check that the file wasn't larger than the buffer size. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Be more explicit about test helpers that conditionally fail the test

c8bb1ed

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm added 3 commits August 12, 2026 21:52

Documentation improvements

a4f323e

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Check the return value of snprintf()

ad7c26c

In case the `%p` output is larger than expected. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Remove now-useless checks

fd4cef7

This block now works even when `length == 0`. Setting `errno = 0` doesn't really help. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm force-pushed the fork-helper-unit-tests-create-framework branch from f598590 to fd4cef7 Compare August 12, 2026 19:53

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

		* \brief Record the current test case as a failure based
		* on a substring search.

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

Uh oh!

Conversation

gilles-peskine-arm commented Apr 3, 2026

PR checklist

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!

Choose a reason for hiding this comment

Uh oh!

bensze01 left a comment

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!

bensze01 Aug 12, 2026 • 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!

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!

gilles-peskine-arm commented Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bensze01 Aug 12, 2026 •

edited

Loading