RFC: Fix builds with MBEDTLS_HAVE_TIME disabled and test #3624

daxtens · 2020-08-31T05:05:40Z

Description

This attempts to bring together #3444 and (an earlier version of) #3449: it adds tests to catch bad uses of time.h when MBEDTLS_HAVE_TIME is not defined.

The idea is to add an additional, overriding include directory that #errors out if time.h is included.

I wanted to get some feedback before I do the changelog and backports.

Status

IN DEVELOPMENT

Requires Backporting

Yes

Which branch? - I'm not sure and would appreciate guidance.

Migrations

NO

Additional comments

Ping @naynajain and @raoulstrackx.

Todos

Tests
Documentation
Changelog updated
Backported

Steps to test or reproduce

Run the tests, specifically build_baremetal and build_crypto_baremetal.

tests/scripts/all.sh

yanesca · 2021-08-09T14:37:52Z

@daxtens I am sorry we somehow lost track of this PR. We would be happy to review it now if you still would like to contribute. Would you like to resolve the conflicts?

daxtens · 2021-08-10T01:20:38Z

Sure, I will add this to my to do list and hopefully get to it within a few days.

daxtens · 2021-08-19T06:12:13Z

This now applies on the development branch and the build_baremetal and build_crypto_baremetal test suites pass on my Linux x86_64 laptop.

yanesca · 2021-08-19T12:11:44Z

Unfortunately on the CI ./tests/scripts/all.sh build_baremetal and ./tests/scripts/all.sh build_crypto_baremetal are failing:

/var/lib/build/tests/include/baremetal-override/time.h:18:2: error: #error "time.h included in a configuration without MBEDTLS_HAVE_TIME"
#error "time.h included in a configuration without MBEDTLS_HAVE_TIME"

Additionally ./tests/scripts/all.sh test_malloc_0_null is failing as well:

In file included from ../../include/mbedtls/build_info.h:59:0,
from ../../include/mbedtls/platform_time.h:25,
from common.h:8,
from common.c:1:
/var/lib/build/tests/configs/config-wrapper-malloc-0-null.h:24:21: error: redefinition of 'custom_calloc'
static inline void *custom_calloc( size_t nmemb, size_t size )

yanesca · 2021-08-19T12:17:14Z

Have you tried running the test suites with ./tests/scripts/all-in-docker.sh? (It supposed to replicate the exact environment on the CI.)

daxtens · 2021-08-19T13:00:13Z

Fascinating. I'll try all-in-docker and report back.

daxtens · 2021-08-20T00:10:40Z

Weird, ./tests/scripts/all-in-docker.sh build_crypto_baremetal and build_baremetal pass for me.

Can you copy out a little more of the context for the first failure please? (the one with the #error from time.h)

I can replicate the failure for test_malloc_0_null. I'll fix that up.

yanesca · 2021-08-20T09:47:36Z

It is weird. Some more context on the error:

  CC    psa_crypto_aead.c
  CC    psa_crypto_driver_wrappers.c
  CC    psa_crypto_mac.c
  CC    oid.c
  CC    psa_crypto_se.c
In file included from /usr/include/x86_64-linux-gnu/sys/types.h:132:0,
                 from /usr/include/stdlib.h:314,
                 from ../include/mbedtls/platform.h:64,
                 from aes.c:33:
/var/lib/build/tests/include/baremetal-override/time.h:18:2: error: #error "time.h included in a configuration without MBEDTLS_HAVE_TIME"
 #error "time.h included in a configuration without MBEDTLS_HAVE_TIME"
  ^
In file included from /usr/include/x86_64-linux-gnu/sys/select.h:43:0,
                 from /usr/include/x86_64-linux-gnu/sys/types.h:219,
                 from /usr/include/stdlib.h:314,
                 from ../include/mbedtls/platform.h:64,
                 from aes.c:33:
/var/lib/build/tests/include/baremetal-override/time.h:18:2: error: #error "time.h included in a configuration without MBEDTLS_HAVE_TIME"
 #error "time.h included in a configuration without MBEDTLS_HAVE_TIME"
  ^
In file included from /usr/include/x86_64-linux-gnu/sys/types.h:132:0,
                 from /usr/include/stdlib.h:314,
                 from ../include/mbedtls/platform.h:64,
                 from aria.c:36:
/var/lib/build/tests/include/baremetal-override/time.h:18:2: error: #error "time.h included in a configuration without MBEDTLS_HAVE_TIME"
 #error "time.h included in a configuration without MBEDTLS_HAVE_TIME"
  ^
In file included from /usr/include/x86_64-linux-gnu/sys/types.h:132:0,
                 from /usr/include/stdlib.h:314,
                 from ../include/mbedtls/threading.h:28,
                 from ecp.c:76:
/var/lib/build/tests/include/baremetal-override/time.h:18:2: error: #error "time.h included in a configuration without MBEDTLS_HAVE_TIME"
 #error "time.h included in a configuration without MBEDTLS_HAVE_TIME"
  ^
In file included from /usr/include/x86_64-linux-gnu/sys/types.h:132:0,
                 from /usr/include/stdlib.h:314,
                 from ../include/mbedtls/platform.h:64,
                 from ssl_cache.c:29:
/var/lib/build/tests/include/baremetal-override/time.h:18:2: error: #error "time.h included in a configuration without MBEDTLS_HAVE_TIME"
 #error "time.h included in a configuration without MBEDTLS_HAVE_TIME"
  ^

And it goes on like that with many more source files.

yanesca · 2021-09-02T12:56:55Z

@daxtens I just learned that ./tests/scripts/all-in-docker.sh doesn't work as expected: it compiles the library on the host and runs the tests in the docker. Can you try manually building and running the test inside the docker image?

daxtens · 2021-09-02T23:04:32Z

oh that's good to know. I will give that a go.

Remove direct inclusion of mbedtls_config.h and replace with build_info.h, as is the convention in Mbed TLS 3.0. Signed-off-by: David Horstmann <[email protected]>

MBEDTLS_HAVE_TIME_ALT implies MBEDTLS_HAVE_TIME, so an extra check for MBEDTLS_HAVE_TIME is not needed. Signed-off-by: David Horstmann <[email protected]>

Change the new baremetal all.sh tests to use $PWD rather than calling pwd again directly. Signed-off-by: David Horstmann <[email protected]>

Signed-off-by: Andrzej Kurek <[email protected]>

AndrzejKurek · 2022-03-04T10:27:46Z

Rebased to have a passing CI.

tom-cosgrove-arm

LGTM

tom-cosgrove-arm · 2022-03-09T14:54:29Z

ping @d3zd3z to review

tom-cosgrove-arm · 2022-03-09T17:43:15Z

Backport (#5563) is approved, so this PR and 5563 are ready to be merged

tom-cosgrove-arm · 2022-03-15T15:52:32Z

CI failures (ci.trustedfirmware.org): 4 x Scripts not permitted to use new java.util.NoSuchElementException java.lang.String + 1 x No such property: pullRequest for class: groovy.lang.Binding

Internal CI all passes

gilles-peskine-arm · 2022-04-04T20:44:47Z

library/timing.c

@@ -157,6 +158,26 @@ int mbedtls_timing_get_delay( void *data )

    return( 0 );
 }
+#else


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 functions mbedtls_timing_get_timer, mbedtls_timing_set_delay and mbedtls_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 require MBEDTLS_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 with MBEDTLS_TIMING_C enabled but not MBEDTLS_HAVE_TIME, and I got ssl-opt.sh to pass. Since #3624, the test cases in ssl-opt.sh that use udp_proxy … pack=50 are failing, because the pack option of the proxy does not work with the dummy implementation of mbedtls_timing_get_timer. The pack 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 if defined(MBEDTLS_TIMING_C). Should this change to … && (defined(MBEDTLS_HAVE_TIME) || defined(MBEDTLS_TIMING_ALT))?

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 either MBEDTLS_HAVE_TIME, or MBEDTLS_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.

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 if HAVE_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.

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

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

I have done that already - with the default config minus HAVE_TIME and HAVE_TIME_DATE, but with TIMING_C, test_suite_ssl and test_suite_timing will not compile, also dtls_server, dtls_client and ssl_client2 from programs will fail to compile.

If you take mbedtls-3.1 and build with default config minus MBEDTLS_HAVE_TIME and MBEDTLS_HAVE_TIME_DATE on a platform that actually has a working time.h, make lib test works but make programs doesn't. The build of the library breaks in this same configuration if time.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.

If you take mbedtls-3.1 and build with default config minus MBEDTLS_HAVE_TIME and MBEDTLS_HAVE_TIME_DATE on a platform that actually has a working time.h, make lib test works but make programs doesn't

So you are removing any implementation (by saying "don't have time or date" and not providing _ALT) but leaving MBEDTLS_TIMING_C set, so this seems reasonable if we say that for MBEDTLS_TIMING_C you need either "have time/date" or _ALT

The build of the library breaks in this same configuration if time.h is not available

Err, I find that the library doesn't try to include a time.h file if I take development and just disable MBEDTLS_HAVE_TIME and MBEDTLS_HAVE_TIME_DATE - let me investigate further

@tom-cosgrove-arm Before this PR (e.g. in 3.1), platform.h included time.h even when MBEDTLS_HAVE_TIME was disabled.

@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 TLS development and disable only MBEDTLS_HAVE_TIME and MBEDTLS_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

raoulstrackx suggested changes Sep 2, 2020

View reviewed changes

tests/scripts/all.sh Outdated Show resolved Hide resolved

tests/scripts/all.sh Outdated Show resolved Hide resolved

gilles-peskine-arm reviewed Sep 2, 2020

View reviewed changes

tests/scripts/all.sh Outdated Show resolved Hide resolved

daxtens force-pushed the timeless branch from 3b36140 to f843ba5 Compare September 2, 2020 13:33

daxtens requested a review from gilles-peskine-arm September 2, 2020 13:37

daxtens force-pushed the timeless branch from f843ba5 to cc03bc1 Compare September 2, 2020 14:01

daverodgman added the needs-review Every commit must be reviewed by at least two team members, label Sep 3, 2020

daverodgman added Community needs-reviewer This PR needs someone to pick it up for review labels Oct 2, 2020

mstarzyk-mobica removed the needs-reviewer This PR needs someone to pick it up for review label Apr 1, 2021

yanesca added the needs-reviewer This PR needs someone to pick it up for review label Aug 9, 2021

yanesca added the needs-work label Aug 9, 2021

daxtens force-pushed the timeless branch from cc03bc1 to 6815a1b Compare August 19, 2021 06:11

gilles-peskine-arm added needs-ci Needs to pass CI tests and removed 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 labels Aug 31, 2021

davidhorstmann-arm and others added 11 commits March 4, 2022 05:07

programs/fuzz: Use build_info.h in common.h

4e0cc40

Remove direct inclusion of mbedtls_config.h and replace with build_info.h, as is the convention in Mbed TLS 3.0. Signed-off-by: David Horstmann <[email protected]>

programs/fuzz: Remove superfluous MBEDTLS_HAVE_TIME

ca53459

MBEDTLS_HAVE_TIME_ALT implies MBEDTLS_HAVE_TIME, so an extra check for MBEDTLS_HAVE_TIME is not needed. Signed-off-by: David Horstmann <[email protected]>

Use $PWD instead of $(pwd) for consistency

61faf66

Change the new baremetal all.sh tests to use $PWD rather than calling pwd again directly. Signed-off-by: David Horstmann <[email protected]>

Add a changelog entry

3475b26

Signed-off-by: Andrzej Kurek <[email protected]>

Add a missing guard for time.h in net_sockets.c

108bf52

Signed-off-by: Andrzej Kurek <[email protected]>

Fix requirement mismatch in fuzz/common.c

06a00af

Signed-off-by: Andrzej Kurek <[email protected]>

Provide a dummy implementation of timing.c

09e803c

Signed-off-by: Andrzej Kurek <[email protected]>

Fix benchmark and udp_proxy dependency on MBEDTLS_HAVE_TIME

6056e7a

Signed-off-by: Andrzej Kurek <[email protected]>

Add the timing test dependency on MBEDTLS_HAVE_TIME

469fa95

Signed-off-by: Andrzej Kurek <[email protected]>

Guard cache_timeout in ssl_server2 with MBEDTLS_HAVE_TIME

554b820

Signed-off-by: Andrzej Kurek <[email protected]>

Refactor ssl_context_info time printing

541318a

Signed-off-by: Andrzej Kurek <[email protected]>

AndrzejKurek dismissed tom-cosgrove-arm’s stale review via 541318a March 4, 2022 10:27

AndrzejKurek force-pushed the timeless branch from 3f0c37e to 541318a Compare March 4, 2022 10:27

tom-cosgrove-arm removed the needs-work label Mar 4, 2022

tom-cosgrove-arm approved these changes Mar 4, 2022

View reviewed changes

AndrzejKurek requested a review from d3zd3z March 4, 2022 18:18

d3zd3z approved these changes Mar 9, 2022

View reviewed changes

tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-backports Backports are missing or are pending review and approval. labels Mar 9, 2022

daverodgman merged commit 2cecd8a into Mbed-TLS:development Mar 15, 2022

gilles-peskine-arm reviewed Apr 4, 2022

View reviewed changes

AndrzejKurek mentioned this pull request Apr 7, 2022

Remove the dependency on MBEDTLS_TIME_H from the timing module #5708

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Fix builds with MBEDTLS_HAVE_TIME disabled and test #3624

RFC: Fix builds with MBEDTLS_HAVE_TIME disabled and test #3624

daxtens commented Aug 31, 2020 •

edited

Loading

yanesca commented Aug 9, 2021

daxtens commented Aug 10, 2021 via email

daxtens commented Aug 19, 2021

yanesca commented Aug 19, 2021 •

edited

Loading

yanesca commented Aug 19, 2021

daxtens commented Aug 19, 2021

daxtens commented Aug 20, 2021

yanesca commented Aug 20, 2021

yanesca commented Sep 2, 2021

daxtens commented Sep 2, 2021 via email

AndrzejKurek commented Mar 4, 2022

tom-cosgrove-arm left a comment

tom-cosgrove-arm commented Mar 9, 2022

tom-cosgrove-arm commented Mar 9, 2022

tom-cosgrove-arm commented Mar 15, 2022 •

edited

Loading

gilles-peskine-arm Apr 4, 2022

AndrzejKurek Apr 5, 2022

mpg Apr 6, 2022

tom-cosgrove-arm Apr 6, 2022

AndrzejKurek Apr 6, 2022 •

edited

Loading

gilles-peskine-arm Apr 6, 2022

tom-cosgrove-arm Apr 6, 2022

tom-cosgrove-arm Apr 6, 2022

gilles-peskine-arm Apr 6, 2022

tom-cosgrove-arm Apr 6, 2022

RFC: Fix builds with MBEDTLS_HAVE_TIME disabled and test #3624

RFC: Fix builds with MBEDTLS_HAVE_TIME disabled and test #3624

Conversation

daxtens commented Aug 31, 2020 • edited Loading

Description

Status

Requires Backporting

Migrations

Additional comments

Todos

Steps to test or reproduce

yanesca commented Aug 9, 2021

daxtens commented Aug 10, 2021 via email

daxtens commented Aug 19, 2021

yanesca commented Aug 19, 2021 • edited Loading

yanesca commented Aug 19, 2021

daxtens commented Aug 19, 2021

daxtens commented Aug 20, 2021

yanesca commented Aug 20, 2021

yanesca commented Sep 2, 2021

daxtens commented Sep 2, 2021 via email

AndrzejKurek commented Mar 4, 2022

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented Mar 9, 2022

tom-cosgrove-arm commented Mar 9, 2022

tom-cosgrove-arm commented Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrzejKurek Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxtens commented Aug 31, 2020 •

edited

Loading

yanesca commented Aug 19, 2021 •

edited

Loading

tom-cosgrove-arm commented Mar 15, 2022 •

edited

Loading

AndrzejKurek Apr 6, 2022 •

edited

Loading