From a77686938a8f86637d283bf2a46d2434f844500e Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Wed, 5 May 2021 16:49:18 -0700 Subject: [PATCH] Remove exposing windows.h header from our public headers and reduce risk of introducing it in non-public headers (#2188) Fixes https://github.com/Azure/azure-sdk-for-cpp/issues/1853 --- sdk/core/azure-core/CMakeLists.txt | 1 - .../azure/core/http/win_http_transport.hpp | 12 ++++ .../inc/azure/core/internal/hkey_holder.hpp | 65 ------------------- .../inc/azure/core/io/body_stream.hpp | 14 +--- sdk/core/azure-core/src/http/curl/curl.cpp | 10 ++- .../azure-core/src/http/telemetry_policy.cpp | 42 +++++++++++- sdk/core/azure-core/src/io/body_stream.cpp | 21 ++++-- .../src/io/random_access_file_body_stream.cpp | 4 +- .../environment_log_level_listener.hpp | 1 + .../src/block_blob_client.cpp | 12 ++++ .../inc/azure/storage/common/file_io.hpp | 2 +- .../azure-storage-common/src/file_io.cpp | 43 ++++++++---- .../azure-storage-common/test/test_base.hpp | 12 ++++ 13 files changed, 139 insertions(+), 100 deletions(-) delete mode 100644 sdk/core/azure-core/inc/azure/core/internal/hkey_holder.hpp diff --git a/sdk/core/azure-core/CMakeLists.txt b/sdk/core/azure-core/CMakeLists.txt index 3a6c37f2c..f1bec922f 100644 --- a/sdk/core/azure-core/CMakeLists.txt +++ b/sdk/core/azure-core/CMakeLists.txt @@ -57,7 +57,6 @@ set( inc/azure/core/internal/client_options.hpp inc/azure/core/internal/contract.hpp inc/azure/core/internal/diagnostics/log.hpp - inc/azure/core/internal/hkey_holder.hpp inc/azure/core/internal/http/pipeline.hpp inc/azure/core/internal/io/null_body_stream.hpp inc/azure/core/internal/json/json_serializable.hpp diff --git a/sdk/core/azure-core/inc/azure/core/http/win_http_transport.hpp b/sdk/core/azure-core/inc/azure/core/http/win_http_transport.hpp index cc7e7a1ab..30b79d787 100644 --- a/sdk/core/azure-core/inc/azure/core/http/win_http_transport.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/win_http_transport.hpp @@ -12,6 +12,18 @@ #include "azure/core/http/http.hpp" #include "azure/core/http/transport.hpp" +#include + +#if defined(AZ_PLATFORM_WINDOWS) +#if !defined(WIN32_LEAN_AND_MEAN) +#define WIN32_LEAN_AND_MEAN +#endif +#if !defined(NOMINMAX) +#define NOMINMAX +#endif +#include +#endif + #include #include #include diff --git a/sdk/core/azure-core/inc/azure/core/internal/hkey_holder.hpp b/sdk/core/azure-core/inc/azure/core/internal/hkey_holder.hpp deleted file mode 100644 index dad089a96..000000000 --- a/sdk/core/azure-core/inc/azure/core/internal/hkey_holder.hpp +++ /dev/null @@ -1,65 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// SPDX-License-Identifier: MIT - -/** - * @file - * @brief Internal HKEY holder. - * - */ -#pragma once - -#include "azure/core/platform.hpp" - -#if defined(AZ_PLATFORM_WINDOWS) -#if !defined(WIN32_LEAN_AND_MEAN) -#define WIN32_LEAN_AND_MEAN -#endif -#if !defined(NOMINMAX) -#define NOMINMAX -#endif - -#include - -#if !defined(WINAPI_PARTITION_DESKTOP) \ - || WINAPI_PARTITION_DESKTOP // See azure/core/platform.hpp for explanation. - -namespace Azure { namespace Core { namespace _internal { - - /** - * @brief HkeyHolder ensures native handle resource is released. - */ - class HkeyHolder { - private: - HKEY m_value = nullptr; - - public: - explicit HkeyHolder() noexcept : m_value(nullptr){}; - - ~HkeyHolder() noexcept - { - if (m_value != nullptr) - { - ::RegCloseKey(m_value); - } - } - - void operator=(HKEY p) noexcept - { - if (p != nullptr) - { - m_value = p; - } - } - - operator HKEY() noexcept { return m_value; } - - operator HKEY*() noexcept { return &m_value; } - - HKEY* operator&() noexcept { return &m_value; } - }; - -}}} // namespace Azure::Core::_internal - -#endif - -#endif diff --git a/sdk/core/azure-core/inc/azure/core/io/body_stream.hpp b/sdk/core/azure-core/inc/azure/core/io/body_stream.hpp index 1f7f14823..1c31f107f 100644 --- a/sdk/core/azure-core/inc/azure/core/io/body_stream.hpp +++ b/sdk/core/azure-core/inc/azure/core/io/body_stream.hpp @@ -12,14 +12,6 @@ #if defined(AZ_PLATFORM_POSIX) #include -#elif defined(AZ_PLATFORM_WINDOWS) -#if !defined(WIN32_LEAN_AND_MEAN) -#define WIN32_LEAN_AND_MEAN -#endif -#if !defined(NOMINMAX) -#define NOMINMAX -#endif -#include #endif #include "azure/core/context.hpp" @@ -171,7 +163,7 @@ namespace Azure { namespace Core { namespace IO { #if defined(AZ_PLATFORM_POSIX) int m_fileDescriptor; #elif defined(AZ_PLATFORM_WINDOWS) - HANDLE m_filehandle; + void* m_filehandle; #endif int64_t m_baseOffset; int64_t m_length; @@ -220,7 +212,7 @@ namespace Azure { namespace Core { namespace IO { * for the necessary duration. The caller is also responsible for closing it once they are * done. */ - RandomAccessFileBodyStream(HANDLE fileHandle, int64_t offset, int64_t length) + RandomAccessFileBodyStream(void* fileHandle, int64_t offset, int64_t length) : m_filehandle(fileHandle), m_baseOffset(offset), m_length(length), m_offset(0) { } @@ -246,7 +238,7 @@ namespace Azure { namespace Core { namespace IO { private: // immutable #if defined(AZ_PLATFORM_WINDOWS) - HANDLE m_filehandle; + void* m_filehandle; #elif defined(AZ_PLATFORM_POSIX) int m_fileDescriptor; #endif diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index 9142a4f59..bd7a2c117 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -17,6 +17,12 @@ #include // for poll() #include // for socket shutdown #elif defined(AZ_PLATFORM_WINDOWS) +#if !defined(WIN32_LEAN_AND_MEAN) +#define WIN32_LEAN_AND_MEAN +#endif +#if !defined(NOMINMAX) +#define NOMINMAX +#endif #include // for WSAPoll(); #endif @@ -803,7 +809,7 @@ int64_t CurlSession::OnRead(uint8_t* buffer, int64_t count, Context const& conte auto totalRead = int64_t(); auto readRequestLength = this->m_isChunkedResponseType - ? std::min(this->m_chunkSize - this->m_sessionTotalRead, count) + ? (std::min)(this->m_chunkSize - this->m_sessionTotalRead, count) : count; // For responses with content-length, avoid trying to read beyond Content-length or @@ -812,7 +818,7 @@ int64_t CurlSession::OnRead(uint8_t* buffer, int64_t count, Context const& conte if (this->m_contentLength > 0) { auto remainingBodyContent = this->m_contentLength - this->m_sessionTotalRead; - readRequestLength = std::min(readRequestLength, remainingBodyContent); + readRequestLength = (std::min)(readRequestLength, remainingBodyContent); } // Take data from inner buffer if any diff --git a/sdk/core/azure-core/src/http/telemetry_policy.cpp b/sdk/core/azure-core/src/http/telemetry_policy.cpp index b65314389..11deb47c3 100644 --- a/sdk/core/azure-core/src/http/telemetry_policy.cpp +++ b/sdk/core/azure-core/src/http/telemetry_policy.cpp @@ -17,7 +17,47 @@ #include -#include "azure/core/internal/hkey_holder.hpp" +#if !defined(WINAPI_PARTITION_DESKTOP) \ + || WINAPI_PARTITION_DESKTOP // See azure/core/platform.hpp for explanation. + +namespace Azure { namespace Core { namespace _internal { + + /** + * @brief HkeyHolder ensures native handle resource is released. + */ + class HkeyHolder { + private: + HKEY m_value = nullptr; + + public: + explicit HkeyHolder() noexcept : m_value(nullptr){}; + + ~HkeyHolder() noexcept + { + if (m_value != nullptr) + { + ::RegCloseKey(m_value); + } + } + + void operator=(HKEY p) noexcept + { + if (p != nullptr) + { + m_value = p; + } + } + + operator HKEY() noexcept { return m_value; } + + operator HKEY*() noexcept { return &m_value; } + + HKEY* operator&() noexcept { return &m_value; } + }; + +}}} // namespace Azure::Core::_internal + +#endif #elif defined(AZ_PLATFORM_POSIX) #include diff --git a/sdk/core/azure-core/src/io/body_stream.cpp b/sdk/core/azure-core/src/io/body_stream.cpp index 7449f7127..554da0bb5 100644 --- a/sdk/core/azure-core/src/io/body_stream.cpp +++ b/sdk/core/azure-core/src/io/body_stream.cpp @@ -33,6 +33,12 @@ using Azure::Core::Context; using namespace Azure::Core::IO; +#if defined(AZ_PLATFORM_WINDOWS) +// For an abundance of caution, adding this as a compile time check since we are using static_cast +// between windows HANDLE and void* to avoid having windows.h headers exposed in public headers. +static_assert(sizeof(void*) >= sizeof(HANDLE), "We must be able to cast HANDLE to void* and back."); +#endif + // Keep reading until buffer is all fill out of the end of stream content is reached int64_t BodyStream::ReadToCount(uint8_t* buffer, int64_t count, Context const& context) { @@ -84,12 +90,12 @@ int64_t MemoryBodyStream::OnRead(uint8_t* buffer, int64_t count, Context const& FileBodyStream::FileBodyStream(const std::string& filename) { #if defined(AZ_PLATFORM_WINDOWS) - + HANDLE fileHandle; try { #if !defined(WINAPI_PARTITION_DESKTOP) \ || WINAPI_PARTITION_DESKTOP // See azure/core/platform.hpp for explanation. - m_filehandle = CreateFile( + fileHandle = CreateFile( filename.data(), GENERIC_READ, FILE_SHARE_READ, @@ -99,7 +105,7 @@ FileBodyStream::FileBodyStream(const std::string& filename) // intended to be sequential from beginning to end. NULL); #else - m_filehandle = CreateFile2( + fileHandle = CreateFile2( std::wstring_convert>().from_bytes(filename).c_str(), GENERIC_READ, FILE_SHARE_READ, @@ -107,21 +113,22 @@ FileBodyStream::FileBodyStream(const std::string& filename) NULL); #endif - if (m_filehandle == INVALID_HANDLE_VALUE) + if (fileHandle == INVALID_HANDLE_VALUE) { throw std::runtime_error("Failed to open file for reading. File name: '" + filename + "'"); } LARGE_INTEGER fileSize; - if (!GetFileSizeEx(m_filehandle, &fileSize)) + if (!GetFileSizeEx(fileHandle, &fileSize)) { throw std::runtime_error("Failed to get size of file. File name: '" + filename + "'"); } + m_filehandle = static_cast(fileHandle); m_randomAccessFileBodyStream = std::make_unique<_internal::RandomAccessFileBodyStream>( _internal::RandomAccessFileBodyStream(m_filehandle, 0, fileSize.QuadPart)); } catch (std::exception&) { - CloseHandle(m_filehandle); + CloseHandle(fileHandle); throw; } @@ -156,7 +163,7 @@ FileBodyStream::~FileBodyStream() #if defined(AZ_PLATFORM_WINDOWS) if (m_filehandle) { - CloseHandle(m_filehandle); + CloseHandle(static_cast(m_filehandle)); m_filehandle = NULL; } #elif defined(AZ_PLATFORM_POSIX) diff --git a/sdk/core/azure-core/src/io/random_access_file_body_stream.cpp b/sdk/core/azure-core/src/io/random_access_file_body_stream.cpp index 457b3b49a..7b85ed6dd 100644 --- a/sdk/core/azure-core/src/io/random_access_file_body_stream.cpp +++ b/sdk/core/azure-core/src/io/random_access_file_body_stream.cpp @@ -52,8 +52,10 @@ int64_t RandomAccessFileBodyStream::OnRead( o.Offset = static_cast(this->m_baseOffset + this->m_offset); o.OffsetHigh = static_cast((this->m_baseOffset + this->m_offset) >> 32); + HANDLE fileHandle = static_cast(this->m_filehandle); + auto result = ReadFile( - this->m_filehandle, + fileHandle, buffer, // at most 4Gb to be read static_cast(std::min( diff --git a/sdk/core/azure-core/src/private/environment_log_level_listener.hpp b/sdk/core/azure-core/src/private/environment_log_level_listener.hpp index 94d7c020c..fb2f988fa 100644 --- a/sdk/core/azure-core/src/private/environment_log_level_listener.hpp +++ b/sdk/core/azure-core/src/private/environment_log_level_listener.hpp @@ -13,6 +13,7 @@ #define NOMINMAX #endif +// This use of windows.h within the header is OK because the header is private and in source only. #include #endif diff --git a/sdk/storage/azure-storage-blobs/src/block_blob_client.cpp b/sdk/storage/azure-storage-blobs/src/block_blob_client.cpp index 4110577db..ffdef4528 100644 --- a/sdk/storage/azure-storage-blobs/src/block_blob_client.cpp +++ b/sdk/storage/azure-storage-blobs/src/block_blob_client.cpp @@ -3,6 +3,18 @@ #include "azure/storage/blobs/block_blob_client.hpp" +#include + +#if defined(AZ_PLATFORM_WINDOWS) +#if !defined(WIN32_LEAN_AND_MEAN) +#define WIN32_LEAN_AND_MEAN +#endif +#if !defined(NOMINMAX) +#define NOMINMAX +#endif +#include +#endif + #include #include #include diff --git a/sdk/storage/azure-storage-common/inc/azure/storage/common/file_io.hpp b/sdk/storage/azure-storage-common/inc/azure/storage/common/file_io.hpp index 390e6655c..bf18c3e46 100644 --- a/sdk/storage/azure-storage-common/inc/azure/storage/common/file_io.hpp +++ b/sdk/storage/azure-storage-common/inc/azure/storage/common/file_io.hpp @@ -21,7 +21,7 @@ namespace Azure { namespace Storage { namespace _internal { #if defined(AZ_PLATFORM_WINDOWS) - using FileHandle = HANDLE; + using FileHandle = void*; #elif defined(AZ_PLATFORM_POSIX) using FileHandle = int; #endif diff --git a/sdk/storage/azure-storage-common/src/file_io.cpp b/sdk/storage/azure-storage-common/src/file_io.cpp index 5a323fd55..f728302e9 100644 --- a/sdk/storage/azure-storage-common/src/file_io.cpp +++ b/sdk/storage/azure-storage-common/src/file_io.cpp @@ -12,6 +12,16 @@ #include #endif +#if defined(AZ_PLATFORM_WINDOWS) +#if !defined(WIN32_LEAN_AND_MEAN) +#define WIN32_LEAN_AND_MEAN +#endif +#if !defined(NOMINMAX) +#define NOMINMAX +#endif +#include +#endif + #include #include #include @@ -22,9 +32,11 @@ namespace Azure { namespace Storage { namespace _internal { #if defined(AZ_PLATFORM_WINDOWS) FileReader::FileReader(const std::string& filename) { + HANDLE fileHandle; + #if !defined(WINAPI_PARTITION_DESKTOP) \ || WINAPI_PARTITION_DESKTOP // See azure/core/platform.hpp for explanation. - m_handle = CreateFile( + fileHandle = CreateFile( filename.data(), GENERIC_READ, FILE_SHARE_READ, @@ -33,35 +45,38 @@ namespace Azure { namespace Storage { namespace _internal { FILE_ATTRIBUTE_NORMAL, NULL); #else - m_handle = CreateFile2( + fileHandle = CreateFile2( std::wstring_convert>().from_bytes(filename).c_str(), GENERIC_READ, FILE_SHARE_READ, OPEN_EXISTING, NULL); #endif - if (m_handle == INVALID_HANDLE_VALUE) + if (fileHandle == INVALID_HANDLE_VALUE) { throw std::runtime_error("failed to open file"); } LARGE_INTEGER fileSize; - BOOL ret = GetFileSizeEx(m_handle, &fileSize); + BOOL ret = GetFileSizeEx(fileHandle, &fileSize); if (!ret) { - CloseHandle(m_handle); + CloseHandle(fileHandle); throw std::runtime_error("failed to get size of file"); } + m_handle = static_cast(fileHandle); m_fileSize = fileSize.QuadPart; } - FileReader::~FileReader() { CloseHandle(m_handle); } + FileReader::~FileReader() { CloseHandle(static_cast(m_handle)); } FileWriter::FileWriter(const std::string& filename) { + HANDLE fileHandle; + #if !defined(WINAPI_PARTITION_DESKTOP) \ || WINAPI_PARTITION_DESKTOP // See azure/core/platform.hpp for explanation. - m_handle = CreateFile( + fileHandle = CreateFile( filename.data(), GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, @@ -70,20 +85,21 @@ namespace Azure { namespace Storage { namespace _internal { FILE_ATTRIBUTE_NORMAL, NULL); #else - m_handle = CreateFile2( + fileHandle = CreateFile2( std::wstring_convert>().from_bytes(filename).c_str(), GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, CREATE_ALWAYS, NULL); #endif - if (m_handle == INVALID_HANDLE_VALUE) + if (fileHandle == INVALID_HANDLE_VALUE) { throw std::runtime_error("failed to open file"); } + m_handle = static_cast(fileHandle); } - FileWriter::~FileWriter() { CloseHandle(m_handle); } + FileWriter::~FileWriter() { CloseHandle(static_cast(m_handle)); } void FileWriter::Write(const uint8_t* buffer, int64_t length, int64_t offset) { @@ -98,7 +114,12 @@ namespace Azure { namespace Storage { namespace _internal { overlapped.OffsetHigh = static_cast(static_cast(offset) >> 32); DWORD bytesWritten; - BOOL ret = WriteFile(m_handle, buffer, static_cast(length), &bytesWritten, &overlapped); + BOOL ret = WriteFile( + static_cast(m_handle), + buffer, + static_cast(length), + &bytesWritten, + &overlapped); if (!ret) { throw std::runtime_error("failed to write file"); diff --git a/sdk/storage/azure-storage-common/test/test_base.hpp b/sdk/storage/azure-storage-common/test/test_base.hpp index 3321d9c19..cc38814df 100644 --- a/sdk/storage/azure-storage-common/test/test_base.hpp +++ b/sdk/storage/azure-storage-common/test/test_base.hpp @@ -3,6 +3,18 @@ #pragma once +#include + +#if defined(AZ_PLATFORM_WINDOWS) +#if !defined(WIN32_LEAN_AND_MEAN) +#define WIN32_LEAN_AND_MEAN +#endif +#if !defined(NOMINMAX) +#define NOMINMAX +#endif +#include +#endif + #include #include #include