gh-Mbed-TLS/TF-PSA-Crypto
Watch
1
0
Fork
You've already forked TF-PSA-Crypto
 0
Code Issues 167 Pull requests 92 Releases 5 Activity

Add metatests for the fork test helper as unit tests #737

Open
gilles-peskine-arm wants to merge 9 commits from gilles-peskine-arm/fork-helper-unit-tests-create-crypto into development
pull from: gilles-peskine-arm/fork-helper-unit-tests-create-crypto
merge into: gh-Mbed-TLS:development
Conversation 4 Commits 9 Files changed 4 +543 -1
gilles-peskine-arm commented 2026-04-03 11:09:06 -05:00 (Migrated from github.com)

Together with its framework prerequisite, resolves https://github.com/Mbed-TLS/mbedtls-framework/issues/295.

I am adding a test suite test_suite_metatest whose sole job is to test code that is in the test framework. Ideally, this test suite should be in the test framework. However we don't currently have a place for unit tests in the test framework, and I don't think it's worth the trouble to add them. So instead I am adding the tests in a consuming branch. They don't need to be backported unless the test code that's being tested starts having different behavior based on the consuming branch.

Needs preceding PR: https://github.com/Mbed-TLS/mbedtls-framework/pull/297

PR checklist

  • changelog not required because: test only
  • framework PR provided https://github.com/Mbed-TLS/mbedtls-framework/pull/297
  • TF-PSA-Crypto development PR here
  • 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
  • tests provided
Together with its framework prerequisite, resolves https://github.com/Mbed-TLS/mbedtls-framework/issues/295. I am adding a test suite `test_suite_metatest` whose sole job is to test code that is in the test framework. Ideally, this test suite should be in the test framework. However we don't currently have a place for unit tests in the test framework, and I don't think it's worth the trouble to add them. So instead I am adding the tests in a consuming branch. They don't need to be backported unless the test code that's being tested starts having different behavior based on the consuming branch. Needs preceding PR: https://github.com/Mbed-TLS/mbedtls-framework/pull/297 ## PR checklist - [x] **changelog** not required because: test only - [x] **framework PR** provided https://github.com/Mbed-TLS/mbedtls-framework/pull/297 - [x] **TF-PSA-Crypto development PR** here - [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 - **tests** provided
bensze01 (Migrated from github.com) reviewed 2026-04-03 11:09:06 -05:00
bjwtaylor (Migrated from github.com) reviewed 2026-04-10 09:12:55 -05:00
bjwtaylor (Migrated from github.com) left a comment

Still need to do a full review, however some initial questions/comments.

Still need to do a full review, however some initial questions/comments.
tests/suites/test_suite_metatest.function
@ -0,0 +1,438 @@
/* BEGIN_HEADER */
bjwtaylor (Migrated from github.com) commented 2026-04-10 08:51:15 -05:00

Do we also need something like
sigset_t set;
sigemptyset(&set);
sigaddset(&set, instructions->signal);
sigprocmask(SIG_UNBLOCK, &set, NULL);
to unblock the signal mask? As it's possible the fork will inherit the masked state with an already existing block and at this point the signal will not terminate correctly? The commit already hints at the fact it could be masked, so is there a reason we don't unblock it?

Do we also need something like sigset_t set; sigemptyset(&set); sigaddset(&set, instructions->signal); sigprocmask(SIG_UNBLOCK, &set, NULL); to unblock the signal mask? As it's possible the fork will inherit the masked state with an already existing block and at this point the signal will not terminate correctly? The commit already hints at the fact it could be masked, so is there a reason we don't unblock it?
tests/suites/test_suite_metatest.function
@ -0,0 +387,4 @@
switch (fault) {
case FORK_FAIL_FORK:
resource = RLIMIT_NPROC;
bjwtaylor (Migrated from github.com) commented 2026-04-10 09:12:22 -05:00

minor: If I've understood correctly this code begins a process which essentially sets RLIMIT_NPROC to 0, with the hope this will cause the fork to fail. Will this work for all cases as it can be bypassed for privileged users and can behave differently under container/runtime setups and do we care? If we do maybe a hook would be better?

minor: If I've understood correctly this code begins a process which essentially sets RLIMIT_NPROC to 0, with the hope this will cause the fork to fail. Will this work for all cases as it can be bypassed for privileged users and can behave differently under container/runtime setups and do we care? If we do maybe a hook would be better?
gilles-peskine-arm (Migrated from github.com) reviewed 2026-04-10 10:22:01 -05:00
tests/suites/test_suite_metatest.function
@ -0,0 +1,438 @@
/* BEGIN_HEADER */
gilles-peskine-arm (Migrated from github.com) commented 2026-04-10 10:22:01 -05:00

This is getting into YAGNI territory, but I've made the signal masking more robust.

This is getting into YAGNI territory, but I've made the signal masking more robust.
gilles-peskine-arm (Migrated from github.com) reviewed 2026-04-10 10:25:05 -05:00
tests/suites/test_suite_metatest.function
@ -0,0 +387,4 @@
switch (fault) {
case FORK_FAIL_FORK:
resource = RLIMIT_NPROC;
gilles-peskine-arm (Migrated from github.com) commented 2026-04-10 10:25:05 -05:00

Resource limits are simple and relatively portable. (I've restricted them to Linux, but I think it's mostly a matter of coercing headers to declare the right things, but also possibly of some OSes not enforcing certain limits.)

Hooking into a system call is more dependent how you build the code and on the runtime environment. I'm not sure if it would work on WSL or Cygwin, for example, whereas resource limits should work (although I haven't verified that, and I don't care all that much if they don't work).

A container shouldn't matter (and can make hooking into system calls harder). Running those tests as a privileged user won't work, but that's not something we would do, so I don't think that matters, but I've added a comment.

Resource limits are simple and relatively portable. (I've restricted them to Linux, but I think it's mostly a matter of coercing headers to declare the right things, but also possibly of some OSes not enforcing certain limits.) Hooking into a system call is more dependent how you build the code and on the runtime environment. I'm not sure if it would work on WSL or Cygwin, for example, whereas resource limits should work (although I haven't verified that, and I don't care all that much if they don't work). A container shouldn't matter (and can make hooking into system calls harder). Running those tests as a privileged user won't work, but that's not something we would do, so I don't think that matters, but I've added a comment.
This pull request has changes conflicting with the target branch.
  • framework
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

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-crypto:gilles-peskine-arm/fork-helper-unit-tests-create-crypto'
git switch 'gilles-peskine-arm/fork-helper-unit-tests-create-crypto'

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 'development'
git merge --no-ff 'gilles-peskine-arm/fork-helper-unit-tests-create-crypto'
git switch 'gilles-peskine-arm/fork-helper-unit-tests-create-crypto'
git rebase 'development'
git switch 'development'
git merge --ff-only 'gilles-peskine-arm/fork-helper-unit-tests-create-crypto'
git switch 'gilles-peskine-arm/fork-helper-unit-tests-create-crypto'
git rebase 'development'
git switch 'development'
git merge --no-ff 'gilles-peskine-arm/fork-helper-unit-tests-create-crypto'
git switch 'development'
git merge --squash 'gilles-peskine-arm/fork-helper-unit-tests-create-crypto'
git switch 'development'
git merge --ff-only 'gilles-peskine-arm/fork-helper-unit-tests-create-crypto'
git switch 'development'
git merge 'gilles-peskine-arm/fork-helper-unit-tests-create-crypto'
git push origin 'development'
Sign in to join this conversation.
Reviewers
No reviewers
bensze01
Labels
Clear labels   
api-break
This issue/PR breaks the API and must wait for a new major version

  
approved
Design and code approved - may be waiting for CI or backports

  
bug
Something isn't working

  
component-docs
Docs / website issues filed here for tracking

  
component-platform
Portability layer and build scripts

  
component-test
Test framework and CI scripts

  
dependencies
Pull requests that update a dependency file

  
deprecation
needs deprecation announcement

  
DO-NOT-MERGE

   
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
This issue is not being actively worked on, but PRs welcome.

  
needs-adoption
stalled PR that someone should pick up and complete

  
needs-backports
Backports are missing or are pending review and approval.

  
needs-ci
Needs to pass CI tests

  
needs-design-approval
Needs design discussion / approval

  
needs-info
An issue or PR which needs further info from the reporter / author

  
needs-preceding-pr
Requires another PR to be merged first

  
needs-review
Every commit must be reviewed by at least two team members

  
needs-reviewer
This PR needs someone to pick it up for review

  
needs-work

   
priority-high
High priority - will be reviewed soon

  
priority-low
Low priority - this may not receive review soon

  
priority-medium
Medium priority - this can be reviewed as time permits

  
priority-scheduled
This PR is big - it will require time to be scheduled for review

  
priority-very-high
Highest priority - prioritise this over other review work

  
resolved-after-1.1
The issue is resolved in development, but still present in 1.1 LTS.

  
single-reviewer
This PR qualifies for having only one reviewer

  
size-l
Estimated task size: large (2w+)

  
size-m
Estimated task size: medium (~1w)

  
size-optimisation

   
size-s
Estimated task size: small (~2d)

  
size-xs
Estimated task size: extra small (a few hours at most)

No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
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/TF-PSA-Crypto!737
No description provided.