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

unknown type name 'mbedtls_ecp_restart_ctx' #2242

Closed
jwhui opened this issue Nov 28, 2018 · 11 comments

Comments

Projects
None yet
5 participants
@jwhui
Copy link

commented Nov 28, 2018

Description

  • Type: Bug
  • Priority: Minor

mbed TLS build:
Version: 2.14.0
Configuration: MBEDTLS_ECP_ALT defined and MBEDTLS_ECP_RESTARTABLE not defined

Expected behavior
Compiles without error.

Actual behavior
error: unknown type name 'mbedtls_ecp_restart_ctx'

Steps to reproduce

  1. Define MBEDTLS_ECP_ALT and do not define MBEDTLS_ECP_RESTARTABLE in config.
  2. Compile library/ecdh.c.

I could simply add typedef void mbedtls_ecp_restart_ctx; to my config to fix the above issue, but is that expected?

@RonEld

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

@jwhui Thank you for reporting this issue!
I believe this issue is very similar to #2163 (but not the same), but for the alternative ECP ( as opposed to the alternative ecdsa and acdh functions in that issue)
Obviously, we wouldn't expect a build failure.
As you can see in ecp.h , your fix suggestion is as expected, in case MBEDTLS_ECP_RESTARTABLE is not defined. However, since you have an alternative implementation of of ECP. all your structs defeinitions should be in your ecp_alt.h file.

You should add typedef void mbedtls_ecp_restart_ctx; to ecp_alt.h and not to your config file.
I don't think this is a bug, since it is a matter of alternative implemetnation, and an alternative implementation can define the structs however it fits the driver.
I would like to hear the opinion on this matter from my colleague @mpg

I hope this answers your question

@jwhui

This comment has been minimized.

Copy link
Author

commented Nov 29, 2018

I find it somewhat odd that ecp_alt.h needs to include a definition for mbedtls_ecp_restart_ctx even when MBEDTLS_ECP_RESTARTABLE is not defined. However, is that what's intended?

@RonEld

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2018

As you can see from my previous comment, mbedtls_ecp_restart_ctx should be defined even if MBEDTLS_ECP_RESTARTABLE is not defined, as it is used in other API, such as ecdh and ecdsa. ecp_alt.h is used to define your alternative implementation structures. Since it is not predictable how every platform will need to define their contexts.
Having said that, I agree that in matters of mbedtls_ecp_restart_ctx, when MBEDTLS_ECP_RESTARTABLE is not defined, it is perhaps not the best design, and we may consider moving the definition referenced earlier, to a location independent on MBEDTLS_ECP_ALT.

@mpg

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

Hi @jwhui and thanks for your report! Sorry for replying so late. After analyzing the issue, I fully agree with my colleague @RonEld:

  • currently you have to add something like typedef void mbedtls_ecp_restart_ctx; somewhere and the ideal place is your ecp_alt.h
  • however, we acknowledge that's a bit of an annoyance, and I can confirm that's one we didn't fully anticipate when designing restartable ECC
  • so I think we'll consider removing the requirement for you to define that type in your ecp_alt.h, probably by moving its definition out of the the MBEDTLS_ECP_ALT guards as @RonEld suggests, as it looks like the simplest way to solve the issue.
@mpg

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

I'm relabeling as a bug, as I think that's a regression: existing ecp_alt code that was working and correct with Mbed TLS 2.13.0 will no longer build with Mbed TLS 2.14.0 (until it's modified to add the type definition). This is a break of our compatibility promises, it should not have happened, and should be fixed.

@mpg mpg added bug and removed question labels Dec 20, 2018

@RonEld

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

@mpg I am not entirely convinced this is a bug.
One can have an alternative implementation for the restartable feature as well, in which case, mbedtls_ecp_restart_ctx will be defined differently.

@RonEld RonEld added the tracking label Dec 20, 2018

@mpg

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

@RonEld I think we ruled out alternative implementations of the restartable feature: https://github.com/ARMmbed/mbedtls/blob/development/include/mbedtls/check_config.h#L111 - when we did that, we should have moved the type out of the ALT guard, as it's not ALTable.

@RonEld

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

@mpg thank for the reminder! ( I believe I even made that change:) )

@ciarmcom ciarmcom added the mirrored label Dec 20, 2018

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

ARM Internal Ref: IOTSSL-2675

@jwhui

This comment has been minimized.

Copy link
Author

commented Dec 21, 2018

@mpg @RonEld, thanks for looking into this!

@RonEld RonEld added the fix available label Jan 7, 2019

@sbutcher-arm

This comment has been minimized.

Copy link
Collaborator

commented Jan 11, 2019

PR's #2334 and #2336 have been merged so this issue can now be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.