Improve fork helper robustness#297
Open
gilles-peskine-arm wants to merge 8 commits intoMbed-TLS:mainfrom
Open
Conversation
gilles-peskine-arm
added
needs-ci
github-project-automation
bot
moved this to In Development
in Roadmap pull requests (new board)
gilles-peskine-arm
force-pushed
the
fork-helper-unit-tests-create-framework
branch
2 times, most recently
from
7f17fc3 to
bc83f4e
Compare
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>
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>
gilles-peskine-arm
force-pushed
the
fork-helper-unit-tests-create-framework
branch
from
bc83f4e to
cd6fb18
Compare
bjwtaylor
reviewed
Apr 8, 2026
|
|
||
| /* 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); |
There was a problem hiding this comment.
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?
Contributor
Author
There was a problem hiding this comment.
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.
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>
gilles-peskine-arm
added
the
needs-review
bjwtaylor
approved these changes
Apr 9, 2026
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves most of #295, except for the metatests which are added in a consuming branch instead: Mbed-TLS/TF-PSA-Crypto#737.
PR checklist