Azure / Azure/azure-sdk-for-cpp

eventhubs: RetryOperation::CalculateExponentialDelay invokes UB and over-sleeps on final attempt

Open
#7,132 0 comments 0 reactions 0 assignees View on GitHub
needs-triage
Dominant language
C++
Stars
205
Forks
172
Avg merge
1d 3h
Merged PRs (30d)
37

Description

## Summary

`Azure::Messaging::EventHubs::_detail::RetryOperation` in `sdk/eventhubs/azure-messaging-eventhubs/src/retry_operation.cpp` has two bugs in its delay math. Both are separate from the scope of #7131, which is narrowly about rethrowing the captured exception when retries are exhausted.

Bug 1 (undefined behavior on first retry): `RetryOperation::Execute` initializes `retryCount = 0` and passes it as `attempt` into `ShouldRetry`. If the first attempt fails (returns `false` or throws a retriable exception), `ShouldRetry` calls `CalculateExponentialDelay(attempt=0, jitterFactor)`. With `attempt = 0`, the shift expression below evaluates `1 << -1`, which is undefined behavior per [expr.shift]. In practice on x86 it tends to mask the shift amount, but the result is implementation-defined at best and can change with optimizer settings or platforms.

Bug 2 (extra backoff sleep after the final attempt): `WasLastAttempt(attempt)` is `attempt >= MaxRetries`. The loop checks `ShouldRetry(..., retryCount, ...)` against the pre-increment `retryCount`, then increments and `sleep_for`s. With `MaxRetries = 3`:

- attempt 1 fails: `WasLastAttempt(0)` is false, so the loop sleeps
- attempt 2 fails: `WasLastAttempt(1)` is false, so the loop sleeps
- attempt 3 fails: `WasLastAttempt(2)` is false, so the loop sleeps, then the loop condition `retryCount < 3` becomes false and the loop exits

The loop always sleeps one extra time before reporting failure, which slows down failure surfacing for callers.

## Motivation

Inside `CalculateExponentialDelay`, the backoff is computed from the raw attempt index without normalization:

```cpp
auto exponentialRetryAfter = m_retryOptions.RetryDelay
* (((attempt - 1) <= beforeLastBit) ? (1 << (attempt - 1))
: (std::numeric_limits::max)());
```

When the first retry passes `attempt = 0`, `attempt - 1` is `-1`, so `1 << (attempt - 1)` is the negative shift that triggers the UB in Bug 1. Separately, `WasLastAttempt` is evaluated against the pre-increment `retryCount` rather than the attempt that is about to run, so the loop performs a sleep even after the last attempt has failed, which is the over-sleep in Bug 2.

## Proposal

Normalize the attempt index inside the delay calculation (treat the first retry as `attempt=1` for the shift, and check `WasLastAttempt` against `retryCount + 1`), or restructure the loop so the sleep only happens before a *subsequent* attempt and never after the last one.

This is a behavior change for every existing consumer of `RetryOperation` (`producer_client`, `consumer_client`, `partition_client`, `processor`, `processor_partition_client`), so it should be applied as its own PR and review.

Context:

- Original Copilot comment: https://github.com/Azure/azure-sdk-for-cpp/pull/7131#discussion_r3276773679
- Existing `RetryOperationTest.ShouldRetryFalse1`/`ShouldRetryFalse2` already exercise the shift-by-negative path.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.