-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix preprocessor guards for C99 format size specifiers #10020
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
Changes from all commits
1221052
c6a8bf0
154066d
58bb7ec
becb21e
ebe1f81
cd1ece7
9ea9504
011b6cb
46e0b1c
24f11a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| Bugfix | ||
| * Fix definition of MBEDTLS_PRINTF_SIZET to prevent runtime crashes that | ||
| occurred whenever SSL debugging was enabled on a copy of Mbed TLS built | ||
| with Visual Studio 2013 or MinGW. | ||
| Fixes #10017. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,16 +108,16 @@ | |
| * | ||
| * This module provides debugging functions. | ||
| */ | ||
| #if (defined(__MINGW32__) && __USE_MINGW_ANSI_STDIO == 0) || (defined(_MSC_VER) && _MSC_VER < 1800) | ||
| #if defined(__MINGW32__) || (defined(_MSC_VER) && _MSC_VER < 1900) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does modern mingw still need the nonstandard definitions? Note that in 3.6, we already require Visual Studio 2017 or above. So in 3.6 and development, the only reason to still support VS 2013 is our own CI scripts, which test with VS 2013 because that's what we want on 2.28. But 2.28 is reaching EOL so that soon won't be a concern anymore.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's what it looks like, at least for the combination of Mingw-w64 8.3.0 and vsnprintf. We have the same check for MinGW in platform.h
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://learn.microsoft.com/en-us/cpp/c-runtime-library/format-specification-syntax-printf-and-wprintf-functions?view=msvc-140#size-prefixes-for-printf-and-wprintf-format-type-specifiers and https://stackoverflow.com/questions/15610053/correct-printf-format-specifier-for-size-t-zu-or-iu/44055215#44055215 confirm that |
||
| #include <inttypes.h> | ||
| #define MBEDTLS_PRINTF_SIZET PRIuPTR | ||
| #define MBEDTLS_PRINTF_LONGLONG "I64d" | ||
| #else \ | ||
| /* (defined(__MINGW32__) && __USE_MINGW_ANSI_STDIO == 0) || (defined(_MSC_VER) && _MSC_VER < 1800) */ | ||
| /* defined(__MINGW32__) || (defined(_MSC_VER) && _MSC_VER < 1900) */ | ||
| #define MBEDTLS_PRINTF_SIZET "zu" | ||
| #define MBEDTLS_PRINTF_LONGLONG "lld" | ||
| #endif \ | ||
| /* (defined(__MINGW32__) && __USE_MINGW_ANSI_STDIO == 0) || (defined(_MSC_VER) && _MSC_VER < 1800) */ | ||
| /* defined(__MINGW32__) || (defined(_MSC_VER) && _MSC_VER < 1900) */ | ||
|
|
||
| #if !defined(MBEDTLS_PRINTF_MS_TIME) | ||
| #include <inttypes.h> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,11 +4,34 @@ | |
| #include "mbedtls/pk.h" | ||
| #include <test/ssl_helpers.h> | ||
|
|
||
| #if defined(_WIN32) | ||
| # include <stdlib.h> | ||
| # include <crtdbg.h> | ||
| #endif | ||
|
|
||
| // Dummy type for builds without MBEDTLS_HAVE_TIME | ||
| #if !defined(MBEDTLS_HAVE_TIME) | ||
| typedef int64_t mbedtls_ms_time_t; | ||
| #endif | ||
|
|
||
| typedef enum { | ||
| PRINTF_SIZET, | ||
| PRINTF_LONGLONG, | ||
| PRINTF_MS_TIME, | ||
| } printf_format_indicator_t; | ||
|
|
||
| const char *const printf_formats[] = { | ||
| [PRINTF_SIZET] = "%" MBEDTLS_PRINTF_SIZET, | ||
| [PRINTF_LONGLONG] = "%" MBEDTLS_PRINTF_LONGLONG, | ||
| [PRINTF_MS_TIME] = "%" MBEDTLS_PRINTF_MS_TIME, | ||
| }; | ||
|
|
||
| struct buffer_data { | ||
| char buf[2000]; | ||
| char *ptr; | ||
| }; | ||
|
|
||
| #if defined(MBEDTLS_SSL_TLS_C) | ||
| static void string_debug(void *data, int level, const char *file, int line, const char *str) | ||
| { | ||
| struct buffer_data *buffer = (struct buffer_data *) data; | ||
|
|
@@ -44,14 +67,77 @@ static void string_debug(void *data, int level, const char *file, int line, cons | |
|
|
||
| buffer->ptr = p; | ||
| } | ||
| #endif /* MBEDTLS_SSL_TLS_C */ | ||
|
|
||
| #if defined(_WIN32) | ||
| static void noop_invalid_parameter_handler( | ||
| const wchar_t *expression, | ||
| const wchar_t *function, | ||
| const wchar_t *file, | ||
| unsigned int line, | ||
| uintptr_t pReserved) | ||
| { | ||
| (void) expression; | ||
| (void) function; | ||
| (void) file; | ||
| (void) line; | ||
| (void) pReserved; | ||
| } | ||
| #endif /* _WIN32 */ | ||
|
|
||
| /* END_HEADER */ | ||
|
|
||
| /* BEGIN_DEPENDENCIES | ||
| * depends_on:MBEDTLS_DEBUG_C:MBEDTLS_SSL_TLS_C | ||
| * depends_on:MBEDTLS_DEBUG_C | ||
| * END_DEPENDENCIES | ||
| */ | ||
|
|
||
| /* BEGIN_CASE */ | ||
| void printf_int_expr(int format_indicator, intmax_t sizeof_x, intmax_t x, char *result) | ||
| { | ||
| #if defined(_WIN32) | ||
| /* Windows treats any invalid format specifiers passsed to the CRT as fatal assertion failures. | ||
| Disable this behaviour temporarily, so the rest of the test cases can complete. */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify, we would only trigger an assertion if the test fails, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, exactly. A failure would cause an assertion, which would terminate the test suite exe immediatly with an error code.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pretty much what we were observing initially and that we were finding quite mysterious. |
||
| _invalid_parameter_handler saved_handler = | ||
| _set_invalid_parameter_handler(noop_invalid_parameter_handler); | ||
|
|
||
| // Disable assertion pop-up window in Debug builds | ||
| int saved_report_mode = _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_REPORT_MODE); | ||
| _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_DEBUG); | ||
| #endif | ||
|
|
||
| const char *format = printf_formats[format_indicator]; | ||
| char *output = NULL; | ||
| const size_t n = strlen(result); | ||
|
|
||
| /* Nominal case: buffer just large enough */ | ||
| TEST_CALLOC(output, n + 1); | ||
| if ((size_t) sizeof_x <= sizeof(int)) { // Any smaller integers would be promoted to an int due to calling a vararg function | ||
| TEST_EQUAL(n, mbedtls_snprintf(output, n + 1, format, (int) x)); | ||
| } else if (sizeof_x == sizeof(long)) { | ||
| TEST_EQUAL(n, mbedtls_snprintf(output, n + 1, format, (long) x)); | ||
| } else if (sizeof_x == sizeof(long long)) { | ||
| TEST_EQUAL(n, mbedtls_snprintf(output, n + 1, format, (long long) x)); | ||
| } else { | ||
| TEST_FAIL( | ||
| "sizeof_x <= sizeof(int) || sizeof_x == sizeof(long) || sizeof_x == sizeof(long long)"); | ||
| } | ||
| TEST_MEMORY_COMPARE(result, n + 1, output, n + 1); | ||
|
|
||
| exit: | ||
| mbedtls_free(output); | ||
| output = NULL; | ||
|
|
||
| #if defined(_WIN32) | ||
| // Restore default Windows behaviour | ||
| _set_invalid_parameter_handler(saved_handler); | ||
| _CrtSetReportMode(_CRT_ASSERT, saved_report_mode); | ||
| (void) saved_report_mode; | ||
| #endif | ||
| } | ||
| /* END_CASE */ | ||
|
|
||
| /* BEGIN_CASE depends_on:MBEDTLS_SSL_TLS_C */ | ||
| void debug_print_msg_threshold(int threshold, int level, char *file, | ||
| int line, char *result_str) | ||
| { | ||
|
|
@@ -89,7 +175,7 @@ exit: | |
| } | ||
| /* END_CASE */ | ||
|
|
||
| /* BEGIN_CASE */ | ||
| /* BEGIN_CASE depends_on:MBEDTLS_SSL_TLS_C */ | ||
| void mbedtls_debug_print_ret(char *file, int line, char *text, int value, | ||
| char *result_str) | ||
| { | ||
|
|
@@ -124,7 +210,7 @@ exit: | |
| } | ||
| /* END_CASE */ | ||
|
|
||
| /* BEGIN_CASE */ | ||
| /* BEGIN_CASE depends_on:MBEDTLS_SSL_TLS_C */ | ||
| void mbedtls_debug_print_buf(char *file, int line, char *text, | ||
| data_t *data, char *result_str) | ||
| { | ||
|
|
@@ -159,7 +245,7 @@ exit: | |
| } | ||
| /* END_CASE */ | ||
|
|
||
| /* BEGIN_CASE depends_on:MBEDTLS_FS_IO:MBEDTLS_X509_CRT_PARSE_C:!MBEDTLS_X509_REMOVE_INFO */ | ||
| /* BEGIN_CASE depends_on:MBEDTLS_SSL_TLS_C:MBEDTLS_FS_IO:MBEDTLS_X509_CRT_PARSE_C:!MBEDTLS_X509_REMOVE_INFO */ | ||
| void mbedtls_debug_print_crt(char *crt_file, char *file, int line, | ||
| char *prefix, char *result_str) | ||
| { | ||
|
|
@@ -199,7 +285,7 @@ exit: | |
| } | ||
| /* END_CASE */ | ||
|
|
||
| /* BEGIN_CASE depends_on:MBEDTLS_BIGNUM_C */ | ||
| /* BEGIN_CASE depends_on:MBEDTLS_SSL_TLS_C:MBEDTLS_BIGNUM_C */ | ||
| void mbedtls_debug_print_mpi(char *value, char *file, int line, | ||
| char *prefix, char *result_str) | ||
| { | ||
|
|
||
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've been wondering: is it really meaningful to have these conditional definitions in a public header? Doesn't it depend on what libc mbedtls is linked against? Which could even be determined at runtime if the application is linked dynamically, which is pretty likely on Windows.
Mind you, in the LTS branches, we probably can't change anything, at most we might want to document any potential problems. And in development, we probably should make parts of
debug.hprivate — those macros are really only intended to be used from TLS, not from applications. Filed as #10054.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.
It should be sufficient to use the macros from
inttypes.heverywhere, now that we have dropped support for Visual Studio 2010, and removed the compat header that shadowedinttypes.h- but transitioning to this strategy in the development feels out of scope for this PR.