Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash in test suite x509write config full no seedfile #6109

Merged

Conversation

superna9999
Copy link
Contributor

@superna9999 superna9999 commented Jul 20, 2022

Description

Steps to reproduce:

find . -name seedfile -exec rm {} +
scripts/config.py config full
(cd tests && make test_suite_x509write && ./test_suite_x509write)

Observed behaviour: the first few tests fail, then eventually there's a bus error and a core dump is produced. If we just add the seedfile again (dd if=/dev/urandom of=./tests/seedfile bs=64 count=1) everything passes and there is no crash.

Cause:
When USE_PSA_INIT() failed because lack of seedfile, mbedtls_x509write_csr_free()
crashed when called on an unitialized mbedtls_x509write_csr struct.

This moves mbedtls_x509write_csr_init before calling USE_PSA_INIT(),
which could probably fail, and use the same flow in x509_csr_check()
and x509_csr_check_opaque().

Resolves #6100

Gatekeeping note: I (mpg) think this should be backported to 2.28, but does not deserve a ChangeLog entry, as the bug is only in test code.

Status

READY

Requires Backporting

Yes 2.28 #6246

Migrations

NO

Additional comments

N/A

Todos

  • Tests

Steps to test or reproduce

test_suite_x509write must not crash without a seedfile

@superna9999 superna9999 force-pushed the 6100-crash-in-test-suite-x509write branch from 4c68188 to e607a74 Compare July 20, 2022 13:55
@superna9999 superna9999 added bug needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review component-test Test framework and CI scripts labels Jul 20, 2022
@superna9999 superna9999 marked this pull request as ready for review July 21, 2022 11:21
@mpg mpg self-requested a review July 21, 2022 11:43
mpg
mpg previously approved these changes Jul 28, 2022
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for figuring out the underlying problem!

@mpg
Copy link
Contributor

mpg commented Jul 28, 2022

Aw, this now has a conflict and will need to be rebased when you come back.

@mpg mpg added needs-work and removed needs-ci Needs to pass CI tests labels Jul 28, 2022
@mpg
Copy link
Contributor

mpg commented Jul 28, 2022

Note: I think this should be backported to 2.28, but does not deserve a ChangeLog entry, as the bug is only in test code.

@mpg mpg added the needs-backports Backports are missing or are pending review and approval. label Jul 28, 2022
Zaya-dyno
Zaya-dyno previously approved these changes Jul 28, 2022
Copy link
Contributor

@Zaya-dyno Zaya-dyno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem does exist.
This PR fixes it. 👍

@mprse
Copy link
Contributor

mprse commented Aug 1, 2022

This one needs to be rebased.

…s_x509write_csr_free() will crash if uninitialized

When USE_PSA_INIT() failed because lack of seedfile, mbedtls_x509write_csr_free()
crashed when called on an unitialized mbedtls_x509write_csr struct.

This moves mbedtls_x509write_csr_init before calling USE_PSA_INIT(),
which could probably fail, and uses the same flow in x509_csr_check()
and x509_csr_check_opaque().

Signed-off-by: Neil Armstrong <[email protected]>
@superna9999 superna9999 dismissed stale reviews from Zaya-dyno and mpg via a97f1ac August 8, 2022 11:55
@superna9999 superna9999 force-pushed the 6100-crash-in-test-suite-x509write branch from e607a74 to a97f1ac Compare August 8, 2022 11:55
@superna9999
Copy link
Contributor Author

Rebased on development to fix merge conflict

Copy link
Contributor

@mprse mprse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@daverodgman daverodgman removed the needs-reviewer This PR needs someone to pick it up for review label Aug 16, 2022
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@mpg mpg removed the needs-review Every commit must be reviewed by at least two team members, label Aug 30, 2022
@mpg mpg added the approved Design and code approved - may be waiting for CI or backports label Aug 30, 2022
@mpg
Copy link
Contributor

mpg commented Aug 30, 2022

@superna9999 Please create a backport to 2.28 so that we can merge this.

@mpg mpg removed the needs-backports Backports are missing or are pending review and approval. label Sep 1, 2022
@mpg mpg merged commit 0777ec1 into Mbed-TLS:development Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports bug component-test Test framework and CI scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash in test suite x509write config full no seedfile
5 participants