Fixed Linux AMQP crash and many other fixes. (#5166)

* Return pointer to message rather than message; cleaned up API surface to remove uAMQP details

* ASAN configuration via CMAKE preset, not environment variable

* clang-tidy fixes

CMakeLists.txt

@@ -1,6 +1,8 @@
 # Copyright (c) Microsoft Corporation.
 # Licensed under the MIT License.
 
+# cspell: words fsanitize
+
 cmake_minimum_required (VERSION 3.13)
 list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake-modules")
 
@@ -120,6 +122,12 @@ if(MSVC_USE_STATIC_CRT AND MSVC)
   endif()
 endif()
 
+# If the cmake presets enable Address Sanitizer, enable it for the project.
+if (ENABLE_ADDRESS_SANITIZER)
+set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fsanitize=address")
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address")
+endif()
+
 if(BUILD_TESTING)
   include(AddGoogleTest)
   enable_testing ()  

CMakePresets.json

@@ -52,6 +52,56 @@
         "rhs": "Windows"
       }
     },
+    {
+      "name": "clang-windows-default",
+      "displayName": "Windows",
+      "description": "Sets Ninja generator, and compilers. Configures vcpkg in manifest mode using clang",
+      "generator": "Ninja",
+      "binaryDir": "${sourceDir}/out/build/${presetName}",
+      "hidden": true,
+      "cacheVariables": {
+        "VCPKG_MANIFEST_MODE": true,
+        "CMAKE_CXX_COMPILER": "clang++.exe",
+        "CMAKE_C_COMPILER": "clang.exe"
+      },
+      "vendor": {
+        "microsoft.com/VisualStudioSettings/CMake/1.0": {
+          "hostOS": [
+            "Windows"
+          ]
+        }
+      },
+      "condition": {
+        "type": "equals",
+        "lhs": "${hostSystemName}",
+        "rhs": "Windows"
+      }
+    },
+    {
+      "name": "gcc-windows-default",
+      "displayName": "Windows",
+      "description": "Sets Ninja generator, and compilers. Configures vcpkg in manifest mode using gcc",
+      "generator": "Ninja",
+      "binaryDir": "${sourceDir}/out/build/${presetName}",
+      "hidden": true,
+      "cacheVariables": {
+        "VCPKG_MANIFEST_MODE": true,
+        "CMAKE_CXX_COMPILER": "g++.exe",
+        "CMAKE_C_COMPILER": "gcc.exe"
+      },
+      "vendor": {
+        "microsoft.com/VisualStudioSettings/CMake/1.0": {
+          "hostOS": [
+            "Windows"
+          ]
+        }
+      },
+      "condition": {
+        "type": "equals",
+        "lhs": "${hostSystemName}",
+        "rhs": "Windows"
+      }
+    },
     {
       "name": "x64-static",
       "displayName": "Windows x64 Static",
@@ -109,6 +159,60 @@
         "strategy": "external"
       }
     },
+    {
+      "name": "x86-clang-static",
+      "displayName": "Windows x86 clang Static",
+      "description": "Windows Default, clang, x86 architecture.",
+      "inherits": "gcc-windows-default",
+      "hidden": true,
+      "cacheVariables": {
+        "VCPKG_TARGET_TRIPLET": "x86-windows-static",
+        "MSVC_USE_STATIC_CRT": true
+      },
+      "architecture": "win32"
+    },
+    {
+      "name": "x64-clang-static",
+      "displayName": "Windows x64 CLANG Static",
+      "description": "Windows Default, clang, x64 architecture.",
+      "inherits": "clang-windows-default",
+      "hidden": true,
+      "cacheVariables": {
+        "VCPKG_TARGET_TRIPLET": "x64-windows-static",
+        "MSVC_USE_STATIC_CRT": true
+      },
+      "architecture": {
+        "value": "x64",
+        "strategy": "external"
+      }
+    },
+    {
+      "name": "x86-gcc-static",
+      "displayName": "Windows x86 gccStatic",
+      "description": "Windows Default, gcc, x86 architecture.",
+      "inherits": "gcc-windows-default",
+      "hidden": true,
+      "cacheVariables": {
+        "VCPKG_TARGET_TRIPLET": "x86-windows-static",
+        "MSVC_USE_STATIC_CRT": true
+      },
+      "architecture": "win32"
+    },
+    {
+      "name": "x64-gcc-static",
+      "displayName": "Windows x64 GCC Static",
+      "description": "Windows Default, gcc, x64 architecture.",
+      "inherits": "gcc-windows-default",
+      "hidden": true,
+      "cacheVariables": {
+        "VCPKG_TARGET_TRIPLET": "x64-windows-static",
+        "MSVC_USE_STATIC_CRT": true
+      },
+      "architecture": {
+        "value": "x64",
+        "strategy": "external"
+      }
+    },
     {
       "name": "x64",
       "displayName": "Windows x64",
@@ -213,9 +317,8 @@
       "displayName": "Enable Address Sanitizer",
       "description": "Enable Address Sanitizer (Hidden). Note: ASAN can be extremely disruptive, when enabling ASAN, make sure you run your application under the debugger.",
       "hidden": true,
-      "environment": {
-        "CXXFLAGS": "-fsanitize=address",
-        "CFLAGS": "-fsanitize=address"
+      "cacheVariables": {
+        "ENABLE_ADDRESS_SANITIZER": true
       }
     },
     {
@@ -370,7 +473,7 @@
         "enable-address-sanitizer"
       ],
       "cacheVariables": {
-        "DISABLE_AZURE_CORE_OPENTELEMETRY":  true
+        "DISABLE_AZURE_CORE_OPENTELEMETRY": true
       }
     },
     {
@@ -421,6 +524,19 @@
         "enable-perf"
       ]
     },
+    {
+      "name": "x64-static-debug-perftests-clang",
+      "displayName": "x64 Debug static With Perf Tests and samples+clang, libcurl+winhttp",
+      "inherits": [
+        "x64-clang-static",
+        "debug-build",
+        "enable-tests",
+        "enable-samples",
+        "enable-perf",
+        "winhttp-transport",
+        "curl-transport"
+      ]
+    },
     {
       "name": "linux-basic-gcc9",
       "displayName": "Linux GCC 9",

sdk/core/azure-core-amqp/CHANGELOG.md

@@ -4,8 +4,12 @@
 
 ### Features Added
 
+- AMQP Value reference counts are now atomic, this fixes several AMQP related crashes.
+
 ### Breaking Changes
 
+- `MessageReceiver` returns a pointer to the received message instead of a copy.
+
 ### Bugs Fixed
 
 - Fixed several memory leaks.

sdk/core/azure-core-amqp/CMakeLists.txt

@@ -36,14 +36,15 @@ if (VENDOR_UAMQP)
   set(use_installed_dependencies ON CACHE BOOL "Use vcpkg dependencies." FORCE)
   set(skip_samples ON CACHE BOOL "Skip building samples" FORCE)
   set(build_as_object_library ON CACHE BOOL "Produce object library" FORCE)
+  set(atomic_refcount ON CACHE BOOL "Use atomic refcount" FORCE)
 
   add_subdirectory(vendor/azure-uamqp-c SYSTEM)
 
   # uAMQP specific compiler settings. Primarily used to disable warnings in the uAMQP codebase.
   if (MSVC)
     target_compile_definitions(uamqp PRIVATE _CRT_SECURE_NO_WARNINGS)
   elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
-    target_compile_options(uamqp PUBLIC -Wno-extra-semi -Wno-gnu-zero-variadic-macro-arguments -Wno-cast-qual -Wno-format-pedantic)
+    target_compile_options(uamqp PUBLIC -Wno-extra-semi -Wno-gnu-zero-variadic-macro-arguments -Wno-cast-qual -Wno-format-pedantic -Wno-c11-extensions)
   else()
     target_compile_options(uamqp PUBLIC -Wno-pedantic -Wno-implicit-fallthrough -Wno-strict-aliasing)
   endif()

sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/common/global_state.hpp

@@ -25,6 +25,12 @@ namespace Azure { namespace Core { namespace Amqp { namespace Common { namespace
 
   class Pollable {
   public:
+    Pollable() = default;
+    Pollable(const Pollable&) = delete;
+    Pollable& operator=(const Pollable&) = delete;
+    Pollable(Pollable&&) = delete;
+    Pollable& operator=(Pollable&&) = delete;
+
     virtual void Poll() = 0;
     virtual ~Pollable() = default;
   };

sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/connection.hpp

@@ -215,7 +215,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal {
      *
      * If no frames are received within the timeout, the connection will be closed.
      */
-    std::chrono::milliseconds IdleTimeout{std::chrono::milliseconds(60000)};
+    std::chrono::milliseconds IdleTimeout{std::chrono::minutes(1)};
 
     /** @brief The maximum frame size for the connection.
      *

sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/connection_string_credential.hpp

@@ -86,6 +86,41 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal {
       }
     }
 
+    /** @brief Copy constructor
+     *
+     * @remarks Note that TokenCredential derived objects are expected to be passed via
+     * std::shared_ptr and thus should never be directly constructed.
+     */
+    ServiceBusSasConnectionStringCredential(const ServiceBusSasConnectionStringCredential& other)
+        = delete;
+
+    /** @brief Copy assignment operator
+     *
+     *       * @remarks Note that TokenCredential derived objects are expected to be passed via
+     std::shared_ptr and thus
+     * should never be directly constructed.
+
+    */
+    ServiceBusSasConnectionStringCredential& operator=(
+        const ServiceBusSasConnectionStringCredential& other)
+        = delete;
+
+    /** @brief Move constructor
+     *
+     * @remarks Note that TokenCredential derived objects are expected to be passed via
+     * std::shared_ptr and thus should never be directly constructed.
+     */
+    ServiceBusSasConnectionStringCredential(
+        ServiceBusSasConnectionStringCredential&& other) noexcept = delete;
+
+    /** @brief Move assignment operator
+     *
+     * @remarks Note that TokenCredential derived objects are expected to be passed via
+     * std::shared_ptr and thus should never be directly constructed.
+     */
+    ServiceBusSasConnectionStringCredential& operator=(
+        ServiceBusSasConnectionStringCredential&& other) noexcept = delete;
+
     /** @brief Destroy a SAS connection string credential. */
     ~ServiceBusSasConnectionStringCredential() override = default;
 
@@ -123,7 +158,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal {
      *
      * @throw Credentials::AuthenticationException Authentication error occurred.
      */
-    virtual Credentials::AccessToken GetToken(
+    Credentials::AccessToken GetToken(
         Credentials::TokenRequestContext const& tokenRequestContext,
         Context const& context) const override;
 

sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/endpoint.hpp

@@ -35,7 +35,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal {
 
     Endpoint(Endpoint const&) = delete;
     Endpoint& operator=(Endpoint const&) = delete;
-    Endpoint& operator=(Endpoint&& other);
+    Endpoint& operator=(Endpoint&& other) noexcept;
     ENDPOINT_INSTANCE_TAG* Release()
     {
       ENDPOINT_INSTANCE_TAG* rv = m_endpoint;

sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/management.hpp

@@ -113,7 +113,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal {
     /**
      * @brief The response message from the operation, if Status is ManagementOperationStatus::Ok.
      */
-    Models::AmqpMessage Message;
+    std::shared_ptr<Models::AmqpMessage> Message;
 
     /**
      * @brief The error code associated with the message, if Status is
@@ -137,8 +137,36 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal {
    */
   class ManagementClient final {
   public:
+    /**
+     * @brief Represents a client used to manage AMQP entities.
+     */
+    ManagementClient() = default;
+
+    /**
+     * @brief Destructor for the ManagementClient class.
+     */
     ~ManagementClient() noexcept = default;
 
+    /**
+     * @brief Copy constructor.
+     */
+    ManagementClient(ManagementClient const&) = default;
+
+    /**
+     * @brief Assignment operator.
+     */
+    ManagementClient& operator=(ManagementClient const&) = default;
+
+    /**
+     * @brief Move constructor.
+     */
+    ManagementClient(ManagementClient&&) = default;
+
+    /**
+     * @brief Move assignment operator.
+     */
+    ManagementClient& operator=(ManagementClient&&) = default;
+
     /**
      * @brief Open the management instance.
      *
@@ -178,7 +206,6 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal {
   private:
     friend class Azure::Core::Amqp::_detail::ManagementClientFactory;
     ManagementClient(std::shared_ptr<_detail::ManagementClientImpl> impl) : m_impl{impl} {}
-
     std::shared_ptr<_detail::ManagementClientImpl> m_impl;
   };
 

sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/message_receiver.hpp

@@ -102,7 +102,7 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal {
         = 0;
     virtual Models::AmqpValue OnMessageReceived(
         MessageReceiver const& receiver,
-        Models::AmqpMessage const& message)
+        std::shared_ptr<Models::AmqpMessage> const& message)
         = 0;
     virtual void OnMessageReceiverDisconnected(Models::_internal::AmqpError const& error) = 0;
   };
@@ -167,15 +167,15 @@ namespace Azure { namespace Core { namespace Amqp { namespace _internal {
      *
      * @return A pair of the received message and the error if any.
      */
-    std::pair<Azure::Nullable<Models::AmqpMessage>, Models::_internal::AmqpError>
+    std::pair<std::shared_ptr<const Models::AmqpMessage>, Models::_internal::AmqpError>
     WaitForIncomingMessage(Context const& context = {});
 
     /** @brief Return if there are messages waiting to be processed.
      *
      * @return A pair of the received message and the error if any. If both values are empty, then
      * no messages are available and the caller should call WaitForIncomingMessage.
      */
-    std::pair<Azure::Nullable<Models::AmqpMessage>, Models::_internal::AmqpError>
+    std::pair<std::shared_ptr<const Models::AmqpMessage>, Models::_internal::AmqpError>
     TryWaitForIncomingMessage();
 
   private: