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.

Sign up for GitHub

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

Jump to bottom

Remove the dependency on MBEDTLS_TIME_H from the timing module #5708

Merged
merged 3 commits into from
Apr 8, 2022
Merged

Remove the dependency on MBEDTLS_TIME_H from the timing module #5708

merged 3 commits into from
Apr 8, 2022

Conversation

AndrzejKurek
Copy link
Contributor

@AndrzejKurek AndrzejKurek commented Apr 7, 2022 •
edited
Loading

Allow the timing module to include time.h if the platform is suitable, even when MBEDTLS_HAVE_TIME is disabled.
Remove the dummy timing implementation.
Follow-up after #3624.

Remove dummy timing implementation
714b660 
Having such implementation might cause issues for those that
expect to have a working implementation.
Having a compile-time error is better in such case.
Signed-off-by: Andrzej Kurek <[email protected]>
@AndrzejKurek AndrzejKurek added enhancement mbed TLS team needs-review Every commit must be reviewed by at least two team members, component-platform Portability layer and build scripts needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Apr 7, 2022
gilles-peskine-arm
gilles-peskine-arm previously approved these changes Apr 7, 2022
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

I'm ok with these changes.

I think there are further changes to make (changelog entry, fixes to udp_proxy) but I won't ask for them here, it'll be easier for me to make a separate PR.

@gilles-peskine-arm gilles-peskine-arm added needs-backports Backports are missing or are pending review and approval. and removed needs-reviewer This PR needs someone to pick it up for review labels Apr 7, 2022
Andrzej Kurek and others added 2 commits April 8, 2022 04:41
@tom-cosgrove-arm
Remove the dependency on MBEDTLS_HAVE_TIME from MBEDTLS_TIMING_C
5735369 
The timing module might include time.h on its own when on 
a suitable platform, even if MBEDTLS_HAVE_TIME is disabled. 


Co-authored-by: Tom Cosgrove <[email protected]>
Signed-off-by: Andrzej Kurek <[email protected]>
@gilles-peskine-arm
Seed the PRNG even if time() isn't available
e756f64 
time() is only needed to seed the PRNG non-deterministically. If it isn't
available, do seed it, but pick a static seed.

Signed-off-by: Andrzej Kurek <[email protected]>
@AndrzejKurek
Copy link
Contributor Author

I'm ok with these changes.

I think there are further changes to make (changelog entry, fixes to udp_proxy) but I won't ask for them here, it'll be easier for me to make a separate PR.

I added the seed fixes, but the packing dependency is the same as it was - TIMING_C will not compile without a functional timer, hence I didn't provide any fixes for that.
As for the changelog entry - what exactly do you want such entry to tell? The old message stated that:

Bugfix

  • Fix compile errors when MBEDTLS_HAVE_TIME is not defined. Add tests
    to catch bad uses of time.h.

I would say that this was vague enough to still stand.

tom-cosgrove-arm
tom-cosgrove-arm approved these changes Apr 8, 2022
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Apr 8, 2022
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Apr 8, 2022
Merged
@AndrzejKurek AndrzejKurek mentioned this pull request Apr 8, 2022
Merged
@AndrzejKurek
Copy link
Contributor Author

Backport: #5718

@AndrzejKurek AndrzejKurek removed the needs-ci Needs to pass CI tests label Apr 8, 2022
@gilles-peskine-arm gilles-peskine-arm merged commit e1730e4 into Mbed-TLS:development Apr 8, 2022
@bruvzg bruvzg mentioned this pull request Dec 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports component-platform Portability layer and build scripts enhancement needs-backports Backports are missing or are pending review and approval.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AndrzejKurek @gilles-peskine-arm @tom-cosgrove-arm