From f9d4d36ad8e8926ed42b122ff97e0d671080f9ab Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Tue, 23 Feb 2021 09:56:00 -0800 Subject: [PATCH] Defend public headers against inclusion of Windows.h (#1719) * Defend public headers against inclusion of Windows.h ... which defines some nasty function-like macros `min` and `max` when `_NOMINMAX` isn't defined. We prevent expansion as a function-like macro by inserting some token(s) between `min`/`max` and the following `(`. Most commonly that means wrapping the entire qualified-name in `()` a la `(std::min)(x, y)`, but an explicit template argument list (`std::min(x, y)`) works as well. * clang-format all the things * Test coverage I assume that the `azure-meow-common` headers are fully covered by the tests for the `azure-meow-woof` SDKs. --- sdk/core/azure-core/inc/azure/core/context.hpp | 8 +++++--- .../inc/azure/core/http/body_stream.hpp | 2 +- sdk/core/azure-core/test/ut/CMakeLists.txt | 1 + sdk/core/azure-core/test/ut/macro_guard.cpp | 15 +++++++++++++++ .../azure-identity/test/ut/CMakeLists.txt | 1 + .../azure-identity/test/ut/macro_guard.cpp | 15 +++++++++++++++ .../test/ut/CMakeLists.txt | 1 + .../test/ut/macro_guard.cpp | 15 +++++++++++++++ sdk/storage/azure-storage-blobs/CMakeLists.txt | 1 + .../azure-storage-blobs/test/ut/macro_guard.cpp | 15 +++++++++++++++ .../azure/storage/common/concurrent_transfer.hpp | 2 +- .../azure-storage-files-datalake/CMakeLists.txt | 1 + .../test/macro_guard.cpp | 15 +++++++++++++++ .../azure-storage-files-shares/CMakeLists.txt | 1 + .../test/macro_guard.cpp | 15 +++++++++++++++ sdk/template/azure-template/test/CMakeLists.txt | 2 +- .../azure-template/test/ut/macro_guard.cpp | 15 +++++++++++++++ 17 files changed, 119 insertions(+), 6 deletions(-) create mode 100644 sdk/core/azure-core/test/ut/macro_guard.cpp create mode 100644 sdk/identity/azure-identity/test/ut/macro_guard.cpp create mode 100644 sdk/keyvault/azure-security-keyvault-keys/test/ut/macro_guard.cpp create mode 100644 sdk/storage/azure-storage-blobs/test/ut/macro_guard.cpp create mode 100644 sdk/storage/azure-storage-files-datalake/test/macro_guard.cpp create mode 100644 sdk/storage/azure-storage-files-shares/test/macro_guard.cpp create mode 100644 sdk/template/azure-template/test/ut/macro_guard.cpp diff --git a/sdk/core/azure-core/inc/azure/core/context.hpp b/sdk/core/azure-core/inc/azure/core/context.hpp index 02f28c517..bb5e8b5b1 100644 --- a/sdk/core/azure-core/inc/azure/core/context.hpp +++ b/sdk/core/azure-core/inc/azure/core/context.hpp @@ -272,7 +272,9 @@ namespace Azure { namespace Core { return time_point() + static_cast(msec); } - explicit ContextSharedState() : CancelAtMsecSinceEpoch(ToMsecSinceEpoch(time_point::max())) {} + explicit ContextSharedState() : CancelAtMsecSinceEpoch(ToMsecSinceEpoch((time_point::max)())) + { + } explicit ContextSharedState( const std::shared_ptr& parent, @@ -330,7 +332,7 @@ namespace Azure { namespace Core { Context WithValue(const std::string& key, ContextValue&& value) const { return Context{std::make_shared( - m_contextSharedState, time_point::max(), key, std::move(value))}; + m_contextSharedState, (time_point::max)(), key, std::move(value))}; } /** @@ -389,7 +391,7 @@ namespace Azure { namespace Core { void Cancel() { m_contextSharedState->CancelAtMsecSinceEpoch - = ContextSharedState::ToMsecSinceEpoch(time_point::min()); + = ContextSharedState::ToMsecSinceEpoch((time_point::min)()); } /** diff --git a/sdk/core/azure-core/inc/azure/core/http/body_stream.hpp b/sdk/core/azure-core/inc/azure/core/http/body_stream.hpp index 978be7cfa..9ab8bc38a 100644 --- a/sdk/core/azure-core/inc/azure/core/http/body_stream.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/body_stream.hpp @@ -263,7 +263,7 @@ namespace Azure { namespace Core { namespace Http { * @param max_length Maximum number of bytes to provide to the readers. */ LimitBodyStream(BodyStream* inner, int64_t max_length) - : m_inner(inner), m_length(std::min(inner->Length(), max_length)) + : m_inner(inner), m_length((std::min)(inner->Length(), max_length)) { } diff --git a/sdk/core/azure-core/test/ut/CMakeLists.txt b/sdk/core/azure-core/test/ut/CMakeLists.txt index fa53dbb74..a707121ef 100644 --- a/sdk/core/azure-core/test/ut/CMakeLists.txt +++ b/sdk/core/azure-core/test/ut/CMakeLists.txt @@ -40,6 +40,7 @@ add_executable ( http.cpp json.cpp logging.cpp + macro_guard.cpp main.cpp match_conditions.cpp md5.cpp diff --git a/sdk/core/azure-core/test/ut/macro_guard.cpp b/sdk/core/azure-core/test/ut/macro_guard.cpp new file mode 100644 index 000000000..93d0d11c3 --- /dev/null +++ b/sdk/core/azure-core/test/ut/macro_guard.cpp @@ -0,0 +1,15 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: MIT + +// Define `min` and `max` as function-like macros before including all public +// headers to ensure that uses of those identifiers are defended against +// expansion as function-like macros. Define `small` as an object-like macro to +// ensure that identifier isn't used at all. Windows.h is badly behaved and +// defines similar macros with these names and we want to ensure the SDK headers +// function even when a naive user includes Windows.h first. +// +#define small FAIL> diff --git a/sdk/identity/azure-identity/test/ut/CMakeLists.txt b/sdk/identity/azure-identity/test/ut/CMakeLists.txt index 6d6fe2d12..913fbebdd 100644 --- a/sdk/identity/azure-identity/test/ut/CMakeLists.txt +++ b/sdk/identity/azure-identity/test/ut/CMakeLists.txt @@ -13,6 +13,7 @@ include(GoogleTest) add_executable ( azure-identity-test + macro_guard.cpp main.cpp simplified_header.cpp ) diff --git a/sdk/identity/azure-identity/test/ut/macro_guard.cpp b/sdk/identity/azure-identity/test/ut/macro_guard.cpp new file mode 100644 index 000000000..b71186ccf --- /dev/null +++ b/sdk/identity/azure-identity/test/ut/macro_guard.cpp @@ -0,0 +1,15 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: MIT + +// Define `min` and `max` as function-like macros before including all public +// headers to ensure that uses of those identifiers are defended against +// expansion as function-like macros. Define `small` as an object-like macro to +// ensure that identifier isn't used at all. Windows.h is badly behaved and +// defines similar macros with these names and we want to ensure the SDK headers +// function even when a naive user includes Windows.h first. +// +#define small FAIL> diff --git a/sdk/keyvault/azure-security-keyvault-keys/test/ut/CMakeLists.txt b/sdk/keyvault/azure-security-keyvault-keys/test/ut/CMakeLists.txt index d8bed34c9..9600fb06e 100644 --- a/sdk/keyvault/azure-security-keyvault-keys/test/ut/CMakeLists.txt +++ b/sdk/keyvault/azure-security-keyvault-keys/test/ut/CMakeLists.txt @@ -13,6 +13,7 @@ include(GoogleTest) add_executable ( azure-security-keyvault-keys-test key_client_test.cpp + macro_guard.cpp main.cpp mocked_transport_adapter_test.hpp telemetry_header_test.cpp diff --git a/sdk/keyvault/azure-security-keyvault-keys/test/ut/macro_guard.cpp b/sdk/keyvault/azure-security-keyvault-keys/test/ut/macro_guard.cpp new file mode 100644 index 000000000..a83ce445b --- /dev/null +++ b/sdk/keyvault/azure-security-keyvault-keys/test/ut/macro_guard.cpp @@ -0,0 +1,15 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: MIT + +// Define `min` and `max` as function-like macros before including all public +// headers to ensure that uses of those identifiers are defended against +// expansion as function-like macros. Define `small` as an object-like macro to +// ensure that identifier isn't used at all. Windows.h is badly behaved and +// defines similar macros with these names and we want to ensure the SDK headers +// function even when a naive user includes Windows.h first. +// +#define small FAIL> diff --git a/sdk/storage/azure-storage-blobs/CMakeLists.txt b/sdk/storage/azure-storage-blobs/CMakeLists.txt index 417dab056..5fddf6c33 100644 --- a/sdk/storage/azure-storage-blobs/CMakeLists.txt +++ b/sdk/storage/azure-storage-blobs/CMakeLists.txt @@ -96,6 +96,7 @@ if(BUILD_TESTING) test/ut/blob_service_client_test.cpp test/ut/block_blob_client_test.cpp test/ut/block_blob_client_test.hpp + test/ut/macro_guard.cpp test/ut/page_blob_client_test.cpp test/ut/page_blob_client_test.hpp test/ut/storage_retry_policy_test.cpp diff --git a/sdk/storage/azure-storage-blobs/test/ut/macro_guard.cpp b/sdk/storage/azure-storage-blobs/test/ut/macro_guard.cpp new file mode 100644 index 000000000..5428b5178 --- /dev/null +++ b/sdk/storage/azure-storage-blobs/test/ut/macro_guard.cpp @@ -0,0 +1,15 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: MIT + +// Define `min` and `max` as function-like macros before including all public +// headers to ensure that uses of those identifiers are defended against +// expansion as function-like macros. Define `small` as an object-like macro to +// ensure that identifier isn't used at all. Windows.h is badly behaved and +// defines similar macros with these names and we want to ensure the SDK headers +// function even when a naive user includes Windows.h first. +// +#define small FAIL> diff --git a/sdk/storage/azure-storage-common/inc/azure/storage/common/concurrent_transfer.hpp b/sdk/storage/azure-storage-common/inc/azure/storage/common/concurrent_transfer.hpp index e849d1878..bcb9b2362 100644 --- a/sdk/storage/azure-storage-common/inc/azure/storage/common/concurrent_transfer.hpp +++ b/sdk/storage/azure-storage-common/inc/azure/storage/common/concurrent_transfer.hpp @@ -36,7 +36,7 @@ namespace Azure { namespace Storage { namespace Details { break; } int64_t chunkOffset = offset + chunkSize * chunkId; - int64_t chunkLength = std::min(length - chunkSize * chunkId, chunkSize); + int64_t chunkLength = (std::min)(length - chunkSize * chunkId, chunkSize); try { transferFunc(chunkOffset, chunkLength, chunkId, numChunks); diff --git a/sdk/storage/azure-storage-files-datalake/CMakeLists.txt b/sdk/storage/azure-storage-files-datalake/CMakeLists.txt index ab2b650d0..f6aa027dc 100644 --- a/sdk/storage/azure-storage-files-datalake/CMakeLists.txt +++ b/sdk/storage/azure-storage-files-datalake/CMakeLists.txt @@ -100,6 +100,7 @@ if(BUILD_TESTING) test/datalake_sas_test.cpp test/datalake_service_client_test.cpp test/datalake_service_client_test.hpp + test/macro_guard.cpp ) target_link_libraries(azure-storage-test PRIVATE azure-storage-files-datalake) diff --git a/sdk/storage/azure-storage-files-datalake/test/macro_guard.cpp b/sdk/storage/azure-storage-files-datalake/test/macro_guard.cpp new file mode 100644 index 000000000..3fd332bba --- /dev/null +++ b/sdk/storage/azure-storage-files-datalake/test/macro_guard.cpp @@ -0,0 +1,15 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: MIT + +// Define `min` and `max` as function-like macros before including all public +// headers to ensure that uses of those identifiers are defended against +// expansion as function-like macros. Define `small` as an object-like macro to +// ensure that identifier isn't used at all. Windows.h is badly behaved and +// defines similar macros with these names and we want to ensure the SDK headers +// function even when a naive user includes Windows.h first. +// +#define small FAIL> diff --git a/sdk/storage/azure-storage-files-shares/CMakeLists.txt b/sdk/storage/azure-storage-files-shares/CMakeLists.txt index 09c20427e..e71190a82 100644 --- a/sdk/storage/azure-storage-files-shares/CMakeLists.txt +++ b/sdk/storage/azure-storage-files-shares/CMakeLists.txt @@ -86,6 +86,7 @@ if(BUILD_TESTING) target_sources( azure-storage-test PRIVATE + test/macro_guard.cpp test/share_client_test.cpp test/share_client_test.hpp test/share_directory_client_test.cpp diff --git a/sdk/storage/azure-storage-files-shares/test/macro_guard.cpp b/sdk/storage/azure-storage-files-shares/test/macro_guard.cpp new file mode 100644 index 000000000..8a28592cd --- /dev/null +++ b/sdk/storage/azure-storage-files-shares/test/macro_guard.cpp @@ -0,0 +1,15 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: MIT + +// Define `min` and `max` as function-like macros before including all public +// headers to ensure that uses of those identifiers are defended against +// expansion as function-like macros. Define `small` as an object-like macro to +// ensure that identifier isn't used at all. Windows.h is badly behaved and +// defines similar macros with these names and we want to ensure the SDK headers +// function even when a naive user includes Windows.h first. +// +#define small FAIL> diff --git a/sdk/template/azure-template/test/CMakeLists.txt b/sdk/template/azure-template/test/CMakeLists.txt index 2d2b0c7b9..850a81d56 100644 --- a/sdk/template/azure-template/test/CMakeLists.txt +++ b/sdk/template/azure-template/test/CMakeLists.txt @@ -13,6 +13,7 @@ include(GoogleTest) add_executable ( azure-template-test + ut/macro_guard.cpp ut/template_test.cpp ) @@ -23,4 +24,3 @@ if (MSVC) endif() gtest_discover_tests(azure-template-test TEST_PREFIX azure-template.) - diff --git a/sdk/template/azure-template/test/ut/macro_guard.cpp b/sdk/template/azure-template/test/ut/macro_guard.cpp new file mode 100644 index 000000000..6b1096d2c --- /dev/null +++ b/sdk/template/azure-template/test/ut/macro_guard.cpp @@ -0,0 +1,15 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: MIT + +// Define `min` and `max` as function-like macros before including all public +// headers to ensure that uses of those identifiers are defended against +// expansion as function-like macros. Define `small` as an object-like macro to +// ensure that identifier isn't used at all. Windows.h is badly behaved and +// defines similar macros with these names and we want to ensure the SDK headers +// function even when a naive user includes Windows.h first. +// +#define small FAIL>