Azure / Azure/azure-sdk-for-cpp

RequestFailedException loses its message when it is copied

Open
#7,272 0 comments 0 reactions 0 assignees View on GitHub
Azure.Core bug Client
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.