fix(core-amqp): closing a uAMQP sender with an unsettled send is a use-after-free

[j7nw4r]

Summary

MessageSender::Close() frees an in-flight send operation without cancelling its transfer, so a disposition that arrives after the close reaches freed memory. The process stops with SIGSEGV. One unsettled send is enough to reproduce this, and concurrency is not necessary. A container run reproduced it 4 times out of 4 on the uAMQP transport.

Motivation

messagesender_close calls indicate_all_messages_as_error (sdk/core/azure-core-amqp/src/impl/uamqp/vendor/azure-uamqp-c/src/message_sender.c:1027). That function fires each on_message_send_complete and then calls async_operation_destroy, but it never cancels message_with_callback->transfer_async_operation. messagesender_send_cancel_handler does cancel it (message_sender.c:1043), so the two teardown paths disagree.

link_detach in the ATTACHED state only sends the detach frame and moves to HALF_ATTACHED_ATTACH_SENT (link.c:1439), so link->pending_deliveries still points at the operation that indicate_all_messages_as_error freed. When the peer settles the message, link_frame_received calls on_delivery_settled with the freed context (link.c:584). That reaches CompletionOperation::OnOperationFn (sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/common/completion_operation.hpp:31), which already took ownership and deleted the object on the first callback. The second call copies a dead std::function and the process stops.

The defect predates PR 7346. It reproduces on that branch and on its merge base 95ff788c5. No CI leg builds the uAMQP transport, so no pipeline covers this path.

Proposal

Cancel transfer_async_operation on the close path, so indicate_all_messages_as_error matches messagesender_send_cancel_handler. Alternatively, remove the delivery from link->pending_deliveries before the operation is freed, so a late disposition finds no context.

Add a regression test that closes a sender while one send is unsettled and then settles the message from the peer. Such a test needs the socket based mock broker, which builds only on Linux and Windows, and it needs a uAMQP configure with -DDISABLE_RUST_IN_BUILD=ON.

Validation

Reproduced in an Ubuntu 22.04 container on linux/arm64 with -DDISABLE_RUST_IN_BUILD=ON -DBUILD_TESTING=ON -DCMAKE_BUILD_TYPE=Debug. The test binary exits 139 on every attempt. The rest of that suite passes, at 215 tests from 24 suites.

[sagar0207]

Reviewing #7346 surfaced two more uAMQP defects in this same sender teardown / failure-path area. Adding them here rather than opening a separate issue, since they sit adjacent to the use-after-free above and one of them interacts with it directly.

Both are pre-existing, but #7346 makes them newly reachable. Before that PR, MessageSenderImpl::Open waited on the caller's context; a root Context never cancels, so WaitForResult returning null was effectively unreachable and its failure branch was dead code. #7346 adds a 60-second default bound plus a PendingOperationRegistry wake on connection death, and both route execution straight into that branch.

Neither is caught by CI, for the same reason this issue isn't: sdk/core/azure-core-amqp/CMakeLists.txt:36 sets USE_RUST_AMQP ON by default, so no pipeline leg builds uAMQP. The vcpkg port forces it (vcpkg/portfile.cmake:39), so port consumers are the affected population.

---

1. Deadlock between the attach-failure cleanup and the polling thread

MessageSenderImpl::Open failure cleanup takes the connection lock and then calls RemovePollable (src/impl/uamqp/amqp/message_sender.cpp:376-380):

auto lock{m_session->GetConnection()->Lock()};
m_link->UnsubscribeFromDetachEvent();
GlobalStateHolder::GlobalStateInstance()->RemovePollable(m_link);

RemovePollable busy-spins waiting for the polling thread to finish its pass (src/common/global_state.cpp:197-202):

std::lock_guard<std::mutex> lock(m_pollablesMutex);
m_pollables.remove(pollable);
while (m_activelyPolling.load())
  ;

But the polling thread runs ConnectionImpl::Poll(), which takes that same connection mutex as its first action (src/impl/uamqp/amqp/connection.cpp:223).

The cycle:

• caller thread — holds the connection mutex, busy-spins for the polling thread to finish
• polling thread — needs the connection mutex to finish

Because it is a busy spin rather than a blocking wait, the symptom is a permanent hang with one core pinned at 100%. The new 60-second bound cannot rescue it: the bound has already fired, and this hang is in the cleanup that runs afterward.

The receiver already documents this exact hazard (src/impl/uamqp/amqp/message_receiver.cpp:451-456):

Note that you cannot hold any connection or link locks when calling AddPollable. This is because the the AddPollable function attempts to lock the pollable and the RemovePollable function blocks until any pollables have completed while holding the pollable lock.

This can result in a deadlock because the polling thread is also going to acquire the connection lock resulting in a deadlock.

The sender's failure path violates that rule.

Suggested fix: call RemovePollable before acquiring the connection lock, then take the lock for the unsubscribe / close / reset sequence.

---

2. Synthetic timeouts reported as caller cancellation, so nothing retries

RetryOperation deliberately never retries caller cancellation (sdk/eventhubs/azure-messaging-eventhubs/src/retry_operation.cpp:74-77):

catch (Azure::Core::OperationCancelledException const&)
{
  throw;
}

Correct for genuine cancellation. But three uAMQP sites throw OperationCancelledException unconditionally whenever the wait returns null:

• src/impl/uamqp/amqp/message_sender.cpp:384-388 (sender open)
• src/impl/uamqp/amqp/message_sender.cpp:451-457 (sender close)
• src/impl/uamqp/amqp/message_receiver.cpp:522-528 (receiver close)

After #7346 a null result no longer implies the caller cancelled — it also means the SDK's own synthetic 60-second deadline expired, which is transient and retryable. Reporting it as cancellation makes RetryOperation rethrow and abandon recovery. That defeats the intent of the bound: a dropped connection now fails in 60 seconds instead of hanging forever, and is then misclassified as unretryable.

The correct handling already exists a few hundred lines away, in MessageSenderImpl::Send, with a comment spelling out exactly why the two cases must differ:

else if (context.IsCancelled())
{ /* ...OperationCancelled... */ }
else
{
  // Only the deadline that this call added expired. The Event Hubs retry
  // policy treats a timeout as transient and a cancel as final, so these
  // two paths must not share a condition.
  /* ...AmqpErrorCondition::TimeoutError, MessageSendStatus::Timeout... */
}

That reasoning was not carried over to open and close.

Suggested fix: at each of the three sites, test the original context.IsCancelled(). Throw cancellation only when it is genuinely true; otherwise surface AmqpErrorCondition::TimeoutError, which IsErrorTransient already classifies as retryable (sdk/eventhubs/azure-messaging-eventhubs/src/eventhubs_utilities.cpp:136).

---

Interaction with the use-after-free in this issue

Worth flagging for whoever picks this up: #7346 widens the window for the UAF described above.

A send that hits the new 60-second bound does not cancel its uAMQP operation — QueueSendInternal submits with timeout 0 and discards the handle (message_sender.cpp:567-590), deliberately, because cancelling can make uAMQP invoke the completion handler twice. So the transfer stays in flight. The Event Hubs recovery path then closes that sender, which reaches messagesender_close → indicate_all_messages_as_error with an unsettled transfer outstanding — precisely the sequence this issue describes.

Before #7346 that send would simply have blocked forever, so the close was never reached. The new bound turns a hang into a close-with-transfer-in-flight on the common path.

Circumstantial support: the regression test added in #7346 (test/ut/message_sender_receiver.cpp, SenderLateSettlementDoesNotCompleteNextSend) has a 6-second sleep before sender.Close() commented "Let the second message settle, so the close finds no operation in flight." That sleep exists to dodge this UAF.

So fixing this issue likely wants to come before, or alongside, any broader rollout of the default operation bound on uAMQP.

Found by code inspection during review of #7346; no standalone repro for the two above, and CI gives no signal since it doesn't build this transport.