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

@@ -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

sdk/core/azure-core/inc/azure/core/cryptography/hash.hpp

@@ -9,6 +9,8 @@
 
 #pragma once
 
+#include "azure/core/azure_assert.hpp"
+
 #include <memory>
 #include <stdexcept>
 #include <stdint.h>
@@ -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<uint8_t> 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);
     }

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.");
     }
 
     /**

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)

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

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);
 }
 

sdk/core/azure-core/test/ut/md5.cpp

@@ -103,15 +103,23 @@ TEST(Md5Hash, ExpectThrow)
   const uint8_t* ptr = reinterpret_cast<const uint8_t*>(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)