Azure / Azure/azure-sdk-for-cpp
RequestFailedException loses its message when it is copied
- Dominant language
- C++
- Stars
- 205
- Forks
- 172
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 37
Description
## 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`):
```cpp
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`):
```cpp
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.
```cpp
#include
#include
#include
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.
Contributor guide
Research direction
Start with sdk/core/azure-core/inc/azure/core/exception.hpp and sdk/core/azure-core/src/exception.cpp, then inspect the raw-response paths in sdk/storage/azure-storage-common/src/storage_exception.cpp and the derived exception headers. Add the proposed copy tests for the string and raw-response constructors, TransportException, and StorageException; done means what() is preserved, including response details, and the changelog notes that consumers must rebuild.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100