From 75d17550f26c05086cc429fac0bb96a34bd40009 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Tue, 18 May 2021 20:10:14 -0700 Subject: [PATCH] Add argument validation to public APIs. Throw or Precondition/assert failure? (#2189) * Add argument validation to public APIs. * Add exception to spell check. * Revert "Add exception to spell check." This reverts commit be54b614cc1427ddb2f154f5927f4e612a4a1e28. * Turn invalid arg excetion to a precondition assert for bodystream read. * User assert in more places within body_stream. * Update valiation for hashing and bodystream and update tests. * Update precondition test for size_t and add null ptr check * Fix clang formatting. * Update changelog and fix up the message for size_t validation in hash. * Address PR feedback. * More PR feedback. * Remove unnecessary comment from test. * Fix clang format. --- sdk/core/azure-core/CHANGELOG.md | 4 +++ .../inc/azure/core/cryptography/hash.hpp | 24 ++++----------- .../azure-core/inc/azure/core/http/http.hpp | 1 + .../inc/azure/core/io/body_stream.hpp | 8 ++++- sdk/core/azure-core/src/io/body_stream.cpp | 4 +++ sdk/core/azure-core/test/ut/bodystream.cpp | 29 +++++++++++++++++-- sdk/core/azure-core/test/ut/md5.cpp | 18 ++++++++---- 7 files changed, 62 insertions(+), 26 deletions(-) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index cf743cfca..588965f39 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -20,6 +20,10 @@ - Do not re-use a libcurl connection to same host but different port. - Ensure uniqueness of `Azure::Core::Uuid` on POSIX platforms. +### Other Changes and Improvements + +- Modified precondition validation of function arguments to now result in assert failures rather than throwing an exception. + ## 1.0.0-beta.8 (2021-04-07) ### New Features diff --git a/sdk/core/azure-core/inc/azure/core/cryptography/hash.hpp b/sdk/core/azure-core/inc/azure/core/cryptography/hash.hpp index 7949963ff..e74008e8b 100644 --- a/sdk/core/azure-core/inc/azure/core/cryptography/hash.hpp +++ b/sdk/core/azure-core/inc/azure/core/cryptography/hash.hpp @@ -9,6 +9,8 @@ #pragma once +#include "azure/core/azure_assert.hpp" + #include #include #include @@ -61,15 +63,8 @@ namespace Azure { namespace Core { namespace Cryptography { */ void Append(const uint8_t* data, std::size_t length) { - if (!data && length != 0) - { - throw std::invalid_argument( - "Length cannot be " + std::to_string(length) + " if the data pointer is null."); - } - if (m_isDone) - { - throw std::runtime_error("Cannot call Append after calling Final()."); - } + AZURE_ASSERT(data || length == 0); + AZURE_ASSERT_MSG(!m_isDone, "Cannot call Append after calling Final()."); OnAppend(data, length); } @@ -84,15 +79,8 @@ namespace Azure { namespace Core { namespace Cryptography { */ std::vector Final(const uint8_t* data, std::size_t length) { - if (!data && length != 0) - { - throw std::invalid_argument( - "Length cannot be " + std::to_string(length) + " if the data pointer is null."); - } - if (m_isDone) - { - throw std::runtime_error("Cannot call Final() multiple times."); - } + AZURE_ASSERT(data || length == 0); + AZURE_ASSERT_MSG(!m_isDone, "Cannot call Final() multiple times."); m_isDone = true; return OnFinal(data, length); } diff --git a/sdk/core/azure-core/inc/azure/core/http/http.hpp b/sdk/core/azure-core/inc/azure/core/http/http.hpp index 33d724b82..eee0fe1a0 100644 --- a/sdk/core/azure-core/inc/azure/core/http/http.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/http.hpp @@ -187,6 +187,7 @@ namespace Azure { namespace Core { namespace Http { explicit Request(HttpMethod httpMethod, Url url, Azure::Core::IO::BodyStream* bodyStream) : Request(httpMethod, std::move(url), bodyStream, true) { + AZURE_ASSERT_MSG(bodyStream, "The bodyStream pointer cannot be null."); } /** 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 6bdb24cb4..2679ea5bb 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 @@ -61,7 +61,8 @@ namespace Azure { namespace Core { namespace IO { */ virtual void Rewind() { - throw std::logic_error( + AZURE_ASSERT_MSG( + false, "The specified BodyStream doesn't support Rewind which is required to guarantee fault " "tolerance when retrying any operation. Consider creating a MemoryBodyStream or " "FileBodyStream, which are rewindable."); @@ -82,6 +83,8 @@ namespace Azure { namespace Core { namespace IO { size_t count, Azure::Core::Context const& context = Azure::Core::Context()) { + AZURE_ASSERT(buffer || count == 0); + context.ThrowIfCancelled(); return OnRead(buffer, count, context); }; @@ -147,6 +150,7 @@ namespace Azure { namespace Core { namespace IO { */ explicit MemoryBodyStream(const uint8_t* data, size_t length) : m_data(data), m_length(length) { + AZURE_ASSERT(data || length == 0); } int64_t Length() const override { return this->m_length; } @@ -193,6 +197,7 @@ namespace Azure { namespace Core { namespace IO { RandomAccessFileBodyStream(int fileDescriptor, int64_t offset, int64_t length) : m_fileDescriptor(fileDescriptor), m_baseOffset(offset), m_length(length), m_offset(0) { + AZURE_ASSERT(fileDescriptor >= 0 && offset >= 0 && length >= 0); } RandomAccessFileBodyStream() : m_fileDescriptor(0), m_baseOffset(0), m_length(0), m_offset(0) @@ -217,6 +222,7 @@ namespace Azure { namespace Core { namespace IO { RandomAccessFileBodyStream(void* fileHandle, int64_t offset, int64_t length) : m_filehandle(fileHandle), m_baseOffset(offset), m_length(length), m_offset(0) { + AZURE_ASSERT(fileHandle && offset >= 0 && length >= 0); } RandomAccessFileBodyStream() : m_filehandle(NULL), m_baseOffset(0), m_length(0), m_offset(0) diff --git a/sdk/core/azure-core/src/io/body_stream.cpp b/sdk/core/azure-core/src/io/body_stream.cpp index d1d216331..cd6ad9884 100644 --- a/sdk/core/azure-core/src/io/body_stream.cpp +++ b/sdk/core/azure-core/src/io/body_stream.cpp @@ -42,6 +42,8 @@ static_assert(sizeof(void*) >= sizeof(HANDLE), "We must be able to cast HANDLE t // Keep reading until buffer is all fill out of the end of stream content is reached size_t BodyStream::ReadToCount(uint8_t* buffer, size_t count, Context const& context) { + AZURE_ASSERT(buffer || count == 0); + size_t totalRead = 0; for (;;) @@ -89,6 +91,8 @@ size_t MemoryBodyStream::OnRead(uint8_t* buffer, size_t count, Context const& co FileBodyStream::FileBodyStream(const std::string& filename) { + AZURE_ASSERT_MSG(filename.size() > 0, "The file name must not be an empty string."); + #if defined(AZ_PLATFORM_WINDOWS) HANDLE fileHandle = INVALID_HANDLE_VALUE; try diff --git a/sdk/core/azure-core/test/ut/bodystream.cpp b/sdk/core/azure-core/test/ut/bodystream.cpp index 420b2636b..528177ef7 100644 --- a/sdk/core/azure-core/test/ut/bodystream.cpp +++ b/sdk/core/azure-core/test/ut/bodystream.cpp @@ -33,7 +33,15 @@ class TestBodyStream final : public BodyStream { TEST(BodyStream, Rewind) { TestBodyStream tb; - EXPECT_THROW(tb.Rewind(), std::logic_error); + +#if defined(NDEBUG) + // Release build won't provide assert msg + ASSERT_DEATH(tb.Rewind(), ""); +#else + ASSERT_DEATH( + tb.Rewind(), + "The specified BodyStream doesn't support Rewind which is required to guarantee fault "); +#endif std::string testDataPath(AZURE_TEST_DATA_PATH); @@ -53,9 +61,26 @@ TEST(BodyStream, Rewind) EXPECT_NO_THROW(ms.Rewind()); } +TEST(BodyStream, BadInput) +{ + TestBodyStream tb; + ASSERT_DEATH(tb.Read(NULL, 1), ""); + ASSERT_DEATH(tb.Read(NULL, 1, Azure::Core::Context::ApplicationContext), ""); + ASSERT_DEATH(tb.ReadToCount(NULL, 1), ""); + ASSERT_DEATH(tb.ReadToCount(NULL, 1, Azure::Core::Context::ApplicationContext), ""); +} + +TEST(MemoryBodyStream, BadInput) { ASSERT_DEATH(MemoryBodyStream(NULL, 1), ""); } + TEST(FileBodyStream, BadInput) { - EXPECT_THROW(Azure::Core::IO::FileBodyStream(""), std::runtime_error); +#if defined(NDEBUG) + // Release build won't provide assert msg + ASSERT_DEATH(FileBodyStream(""), ""); +#else + ASSERT_DEATH(FileBodyStream(""), "The file name must not be an empty string."); +#endif + EXPECT_THROW(Azure::Core::IO::FileBodyStream("FileNotFound"), std::runtime_error); } diff --git a/sdk/core/azure-core/test/ut/md5.cpp b/sdk/core/azure-core/test/ut/md5.cpp index 3135fe199..7d62462a3 100644 --- a/sdk/core/azure-core/test/ut/md5.cpp +++ b/sdk/core/azure-core/test/ut/md5.cpp @@ -103,15 +103,23 @@ TEST(Md5Hash, ExpectThrow) const uint8_t* ptr = reinterpret_cast(data.c_str()); Md5Hash instance; - EXPECT_THROW(instance.Final(nullptr, 1), std::invalid_argument); - EXPECT_THROW(instance.Append(nullptr, 1), std::invalid_argument); + ASSERT_DEATH(instance.Final(nullptr, 1), ""); + ASSERT_DEATH(instance.Append(nullptr, 1), ""); EXPECT_EQ( Azure::Core::Convert::Base64Encode(instance.Final(ptr, data.length())), "1B2M2Y8AsgTpgAmY7PhCfg=="); - EXPECT_THROW(instance.Final(), std::runtime_error); - EXPECT_THROW(instance.Final(ptr, data.length()), std::runtime_error); - EXPECT_THROW(instance.Append(ptr, data.length()), std::runtime_error); + +#if defined(NDEBUG) + // Release build won't provide assert msg + ASSERT_DEATH(instance.Final(), ""); + ASSERT_DEATH(instance.Final(ptr, data.length()), ""); + ASSERT_DEATH(instance.Append(ptr, data.length()), ""); +#else + ASSERT_DEATH(instance.Final(), "Cannot call Final"); + ASSERT_DEATH(instance.Final(ptr, data.length()), "Cannot call Final"); + ASSERT_DEATH(instance.Append(ptr, data.length()), "Cannot call Append after calling Final"); +#endif } TEST(Md5Hash, CtorDtor)