-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
RFC: Fix builds with MBEDTLS_HAVE_TIME disabled and test #3624
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
09e34b7
Add header guard around malloc(0) returning NULL implementation
daxtens f071024
Do not include time.h without MBEDTLS_HAVE_TIME
daxtens 446af20
tests: prevent inclusion of time.h in baremetal compiles
daxtens 9ed9bc9
programs/ssl: Fix compile errors when MBEDTLS_HAVE_TIME is not defined
raoulstrackx 814c813
tests: add baremetal full config build
daxtens 5b9cb9e
programs/test: fix build without MBEDTLS_HAVE_TIME
davidhorstmann-arm 4e0cc40
programs/fuzz: Use build_info.h in common.h
davidhorstmann-arm ca53459
programs/fuzz: Remove superfluous MBEDTLS_HAVE_TIME
davidhorstmann-arm 61faf66
Use $PWD instead of $(pwd) for consistency
davidhorstmann-arm 3475b26
Add a changelog entry
108bf52
Add a missing guard for time.h in net_sockets.c
06a00af
Fix requirement mismatch in fuzz/common.c
09e803c
Provide a dummy implementation of timing.c
6056e7a
Fix benchmark and udp_proxy dependency on MBEDTLS_HAVE_TIME
469fa95
Add the timing test dependency on MBEDTLS_HAVE_TIME
554b820
Guard cache_timeout in ssl_server2 with MBEDTLS_HAVE_TIME
541318a
Refactor ssl_context_info time printing
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Bugfix | ||
* Fix compile errors when MBEDTLS_HAVE_TIME is not defined. Add tests | ||
to catch bad uses of time.h. |
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
/* | ||
* Copyright The Mbed TLS Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
#error "time.h included in a configuration without MBEDTLS_HAVE_TIME" |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
09e803c introduces a dummy implementation of timer functions. Now, when
defined(MBEDTLS_TIMING_C) && !defined(MBEDTLS_TIMING_ALT) && !defined(MBEDTLS_HAVE_TIME)
(did I get this condition right?), the functionsmbedtls_timing_get_timer
,mbedtls_timing_set_delay
andmbedtls_timing_get_delay
exist but they pretend that no time ever elapses.What is the point of this dummy implementation? Why not make
MBEDTLS_TIMING_C
requireMBEDTLS_HAVE_TIME || MBEDTLS_TIMING_ALT
, since this seems to be necessary to implement the timing functions?The dummy implementation breaks #5582 and I'm trying to figure out how to fix this. Before #3624, in #5582, I made a configuration
config-ccm-psk-dtls1_2.h
withMBEDTLS_TIMING_C
enabled but notMBEDTLS_HAVE_TIME
, and I gotssl-opt.sh
to pass. Since #3624, the test cases inssl-opt.sh
that useudp_proxy … pack=50
are failing, because thepack
option of the proxy does not work with the dummy implementation ofmbedtls_timing_get_timer
. Thepack
options causes the proxy to accumulate packets until a certain amount of time has elapsed, but with the dummy implementation, this never happens, so the proxy never forwards any packet.Currently, in
udp_proxy.c
,opt.pack
is only accepted ifdefined(MBEDTLS_TIMING_C)
. Should this change to… && (defined(MBEDTLS_HAVE_TIME) || defined(MBEDTLS_TIMING_ALT))
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I recall the discussion correctly (and looking at
mbedtls_config.h
),MBEDTLS_TIMING_C
itself provides only the interface that is to be filled by eitherMBEDTLS_HAVE_TIME
, orMBEDTLS_TIMING_ALT
.I would assume that if someone wants to have the interface, but does not wish to provide any working stub - this is the way to go. I don't know why would this be desirable, but that's the way it works now.
It might make sense to go a step further, add dependencies for
MBEDTLS_TIMING_C
as you mentioned and get rid of the dummy code, but this was out of scope for this PR.For now, yes, to have a working implementation,
(defined(MBEDTLS_HAVE_TIME) || defined(MBEDTLS_TIMING_ALT))
is the way to go.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to think those three functions should depend on
defined(MBEDTLS_HAVE_TIME) || defined(MBEDTLS_TIMING_ALT)
and not be declared otherwise. (Just like for example,mbedtls_ssl_cache_set_timeout()
is only declared ifHAVE_TIME
is defined.) Not being defined seems better than having a dummy implementation that's not suitable for any use: when something doesn't work, it's better for the error to be at compile-time than at runtime.Now, whether that's a change we can make now without breaking backwards compat as another question. If the difference only happens in builds that were anyway not working in the previous release, then we're good, otherwise we're stuck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we take the dummy implementation out there will be several compile failures - it's on my list to check, hopefully later today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done that already - with the default config minus
HAVE_TIME
andHAVE_TIME_DATE
, but withTIMING_C
,test_suite_ssl
andtest_suite_timing
will not compile, alsodtls_server
,dtls_client
andssl_client2
fromprograms
will fail to compile.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you take mbedtls-3.1 and build with default config minus
MBEDTLS_HAVE_TIME
andMBEDTLS_HAVE_TIME_DATE
on a platform that actually has a workingtime.h
,make lib test
works butmake programs
doesn't. The build of the library breaks in this same configuration iftime.h
is not available — which was the purpose of this PR.Is it acceptable for us to break custom configurations that accidentally work because the developer didn't include
MBEDTLS_HAVE_TIME
in the configuration, but could have? If we do, at the very least, this should be prominently mentioned in the changelog entry.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you are removing any implementation (by saying "don't have time or date" and not providing
_ALT
) but leavingMBEDTLS_TIMING_C
set, so this seems reasonable if we say that forMBEDTLS_TIMING_C
you need either "have time/date" or_ALT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err, I find that the library doesn't try to include a
time.h
file if I takedevelopment
and just disableMBEDTLS_HAVE_TIME
andMBEDTLS_HAVE_TIME_DATE
- let me investigate furtherThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tom-cosgrove-arm Before this PR (e.g. in 3.1),
platform.h
includedtime.h
even whenMBEDTLS_HAVE_TIME
was disabled.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gilles-peskine-arm So I have double-checked: if I take a working Ubuntu build system, rename
time.h
out of the way, take Mbed TLSdevelopment
and disable onlyMBEDTLS_HAVE_TIME
andMBEDTLS_HAVE_TIME_DATE
, the whole thing builds (as you say, that was the purpose of the PR). i.e. I can't reproduce your failure