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

constant_time.h: add #ifdef __cplusplus guard #9127

Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

constant_time.h: add #ifdef __cplusplus guard #9127

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions include/mbedtls/constant_time.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
#ifndef MBEDTLS_CONSTANT_TIME_H
#define MBEDTLS_CONSTANT_TIME_H

Copy link
Contributor

Choose a reason for hiding this comment

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

We currently validate C++ compatibility only with by compiling a trivial program that includes all headers but does nothing. This rejects, for example, extern "C" { without the matching }, but doesn't detect if it's misplaced or missing. Can we do better and catch a missing or misplaced extern "C", without going through the trouble of calling every function?

Copy link
Author

@nbfalcon nbfalcon May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

An idea would be to add a lint to CI that:

  • Has a list of "important" headers that actually declare functions. I think there are some macro-only header-files, which don't need extern C.
  • Greps each file in the list for the exact multi-line strings
#ifdef __cplusplus
extern "C" {
#endif

I can extend the script you linked if you'd like.

Copy link
Author

Choose a reason for hiding this comment

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

These are the errors generated currently for ref. We'd need some more elegant way to mark the headers that are to be excluded.

nikita@fedora-3:~/src/mbedtls$ ./programs/test/generate_cpp_dummy_build.sh
Error: include/mbedtls/build_info.h does not contain a __cplusplus guard
Error: include/mbedtls/check_config.h does not contain a __cplusplus guard
Error: include/mbedtls/compat-2.x.h does not contain a __cplusplus guard
Error: include/mbedtls/private_access.h does not contain a __cplusplus guard
Error: include/mbedtls/psa_util.h does not contain a __cplusplus guard
Error: include/psa/build_info.h does not contain a __cplusplus guard
Error: include/psa/crypto_adjust_auto_enabled.h does not contain a __cplusplus guard
Error: include/psa/crypto_adjust_config_key_pair_types.h does not contain a __cplusplus guard
Error: include/psa/crypto_adjust_config_synonyms.h does not contain a __cplusplus guard
Error: include/psa/crypto_builtin_composites.h does not contain a __cplusplus guard
Error: include/psa/crypto_builtin_key_derivation.h does not contain a __cplusplus guard
Error: include/psa/crypto_builtin_primitives.h does not contain a __cplusplus guard
Error: include/psa/crypto_driver_common.h does not contain a __cplusplus guard
Error: include/psa/crypto_driver_contexts_composites.h does not contain a __cplusplus guard
Error: include/psa/crypto_driver_contexts_key_derivation.h does not contain a __cplusplus guard
Error: include/psa/crypto_driver_contexts_primitives.h does not contain a __cplusplus guard
Error: include/psa/crypto_legacy.h does not contain a __cplusplus guard
Error: include/psa/crypto_platform.h does not contain a __cplusplus guard
Error: include/psa/crypto_sizes.h does not contain a __cplusplus guard
Error: include/psa/crypto_types.h does not contain a __cplusplus guard
Error: include/psa/crypto_values.h does not contain a __cplusplus guard
Some headers are missing __cplusplus guards

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking! The only testing we're currently doing is compiling a generated program in one configuration. (Testing more configurations would be good, but out of scope here.)

Several headers are skipped because they aren't meant to be included directory. In addition, some headers only declare macros (*/build_info.h, */private_access.h). That leaves mbedtls/compat-2.x.h (can't be very important since nobody's complained) and mbedtls/psa_util.h (a recent addition). Would you mind adding guards to those as well while you're at it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I won't ask for a non-regression test because this is a long-standing issue and we don't add headers often. But if you're willing to dive into it, linting is done from the component_check_* functions in tests/scripts/all.sh.

Copy link
Contributor

Choose a reason for hiding this comment

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

I consider this a bug in 3.28 and 3.6, so I would prefer to backport it. However, we maintain ABI compatibility in LTS branches. We've never explicitly stated if that's just about the ABI itself, or also encompasses the ABI as seen through C++ linkage. Would adding extern "C" be considered an ABI break? I think not, since it doesn't change the generated binary, but maybe I'm missing something? What about a library that was written in C++ and built against Mbed TLS 3.6.0: would this patch break it if it's applied to 3.6.1?

Copy link
Author

@nbfalcon nbfalcon May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No, this does not affect the ABI at all, since the function definition itself is still written in C and compiled as such (i.e. not compiled by g++). The only problem was that the C++ declaration was wrong, causing a linker error.

I noticed this myself in a platformio project where I included this library (as a git submodule, not the outdated package), and found a strange link error, where only mbedtls_ct_memcmp wasn't found.

As for C++ linkage: well including this file without wrapping it in extern "C" just causes a link error, so there are no well-formed programs that could break by this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the information! Then please make backports to the mbedtls-3.6 and mbedtls-2.28 long-time support branches.

#ifdef __cplusplus
extern "C" {
#endif

#include <stddef.h>

/** Constant-time buffer comparison without branches.
Expand All @@ -33,4 +37,8 @@ int mbedtls_ct_memcmp(const void *a,
const void *b,
size_t n);

#ifdef __cplusplus
}
#endif

#endif /* MBEDTLS_CONSTANT_TIME_H */