RequestFailedException loses its message when it is copied

[j7nw4r]

Summary

Azure::Core::RequestFailedException builds a copy from its Message field, and not from the result of what(). The string constructor never sets Message. A copy of an exception that the string constructor built therefore returns an empty string from what().

Each class that derives from RequestFailedException inherits the defect. Azure::Storage::StorageException is one example.

Motivation

Two functions produce the defect together.

The string constructor sets the base class and leaves Message empty (sdk/core/azure-core/src/exception.cpp:20-22):

RequestFailedException::RequestFailedException(std::string const& what) : std::runtime_error(what)
{
}

The copy constructor builds the base class from other.Message (sdk/core/azure-core/inc/azure/core/exception.hpp:144-153):

RequestFailedException(const RequestFailedException& other)
    : std::runtime_error(other.Message),
      RawResponse(...),
      StatusCode(other.StatusCode), ReasonPhrase(other.ReasonPhrase),
      ClientRequestId(other.ClientRequestId), RequestId(other.RequestId),
      ErrorCode(other.ErrorCode), Message(other.Message)

A copy therefore replaces the text of what() with the text of Message.

A minimal reproduction

This program mirrors the two functions above. It needs no build of the SDK.

#include <cstdio>
#include <stdexcept>
#include <string>

class RequestFailedException : public std::runtime_error {
public:
  std::string Message;

  // exception.cpp:20-22 -- this constructor does not set Message.
  explicit RequestFailedException(std::string const& what) : std::runtime_error(what) {}

  // exception.hpp:144-153 -- this constructor reads other.Message, not other.what().
  RequestFailedException(const RequestFailedException& other)
      : std::runtime_error(other.Message), Message(other.Message)
  {
  }
};

int main()
{
  RequestFailedException original("connection reset by peer");
  RequestFailedException copy(original);
  std::printf("original.what() = \"%s\"\n", original.what());
  std::printf("copy.what()     = \"%s\"\n", copy.what());
  return 0;
}

The output shows the loss:

original.what() = "connection reset by peer"
copy.what()     = ""

The raw response path also changes

The defect is not limited to an empty result. StorageException::CreateFromResponse gives the two fields different text (sdk/storage/azure-storage-common/src/storage_exception.cpp:158-167). It builds what() from the status code, the reason phrase, the service message, and the request ID. It sets Message to the service message alone. A copy of that exception removes the status code, the reason phrase, and the request ID from what().

The RequestFailedException raw response constructor has the same shape (sdk/core/azure-core/src/exception.cpp:24-37). It builds the base class from GetRawResponseErrorMessage, and it sets Message from the message field of the response body. A response body without a message field gives an empty Message, and a copy then gives an empty what().

When a copy occurs

A copy occurs when a caller catches the exception by value, when code throws a named exception object, or when code stores the exception for later use. A search of this repository finds no catch by value site today. The defect therefore affects consumer code and new SDK code, and not a current SDK code path. This is the reason that this issue is not a release blocker.

The types that the defect reaches

RequestFailedException is the only exception type in this repository with a hand-written copy constructor. Each other type uses the implicit copy constructor, and each of those types copies correctly.

• Azure::Core::Http::TransportException derives from RequestFailedException (sdk/core/azure-core/inc/azure/core/http/http.hpp:57). It inherits the defect.
• Azure::Storage::StorageException derives from RequestFailedException (sdk/storage/azure-storage-common/inc/azure/storage/common/storage_exception.hpp:19). It inherits the defect.
• Azure::Core::OperationCancelledException derives from std::runtime_error, and it holds no separate message field (sdk/core/azure-core/inc/azure/core/context.hpp:32). It copies correctly.
• Azure::Core::Credentials::AuthenticationException derives from std::exception, and it holds the text in std::string m_what (sdk/core/azure-core/inc/azure/core/credentials/credentials.hpp:139-158). It copies correctly.

Related work

#7273 adds a new Azure Core exception type for a protocol that is not HTTP, and it derives Azure::Messaging::EventHubs::EventHubsException from that new type. The new type must not repeat this defect. EventHubsException copies correctly today, because it derives from std::runtime_error and it declares no copy constructor (sdk/eventhubs/azure-messaging-eventhubs/inc/azure/messaging/eventhubs/eventhubs_exception.hpp:19).

An alternative that this issue does not propose

The string constructor could set Message from its what parameter. That change makes the two fields hold the same text, and the documentation of Message states a different purpose: it holds the message that the service returned in the HTTP response. The copy constructor is the correct place for the fix.

Proposal

• Build the base class from other.what() in the copy constructor (sdk/core/azure-core/inc/azure/core/exception.hpp:144-153).
• Add a test that copies an exception from the string constructor. The test compares what() before the copy and after the copy.
• Add a test that copies an exception from the raw response constructor, and that makes the same comparison.
• Add a copy test for TransportException and for StorageException. Both types inherit the defect from the base class.

The copy constructor is inline in an installed header, so a consumer who links a prebuilt binary must rebuild to get the corrected behavior. Record that expectation in the change log entry.

Validation

• what() returns the same text before a copy and after a copy, for each constructor.
• The new tests fail against the current code, and they pass after the change.
• StorageException keeps the status code, the reason phrase, and the request ID in what() after a copy.
• TransportException keeps the text of what() after a copy.