Skip to content

Heap Use-After-Free in FDv1AdapterSynchronizer (Server-Side C++ SDK, FDv2 → FDv1 Fallback Path) #602

Open

Description

@TerryJLU

Summary

FDv1AdapterSynchronizer wraps a legacy FDv1 IDataSynchronizer
(StreamingDataSource or PollingDataSource) so it can serve the
newer FDv2 orchestrator's pull-based interface. This is the object the
server SDK constructs whenever it falls back from the FDv2 protocol to
FDv1 — an entirely ordinary, documented, production code path (e.g.
triggered by an X-LD-FD-Fallback directive from the LaunchDarkly
streaming/polling endpoint).

FDv1AdapterSynchronizer::Close() discards the completion callback of
the wrapped source's asynchronous ShutdownAsync() call:

void FDv1AdapterSynchronizer::Close() {
    if (!close_promise_.Resolve(std::monostate{})) { return; }
    std::lock_guard lock(lifecycle_mutex_);
    bool const was_started = started_;
    started_ = true;
    if (was_started) {
        fdv1_source_->ShutdownAsync([] {});   // <-- completion discarded
    }
}

IDataSynchronizer's own interface contract
(libs/server-sdk/src/data_interfaces/source/idata_synchronizer.hpp:34)
states that the destination pointer handed to it via StartAsync is
"invalid after the ShutdownAsync completion handler is called" —
i.e., callers must wait for that signal before destroying the
destination. Close() never stores or awaits this callback; it fires
a no-op lambda and returns immediately. ~FDv1AdapterSynchronizer()
then proceeds to destroy destination_ (a
std::unique_ptr<ConvertingDestination>) and status_manager_ (a
DataSourceStatusManager, shared by reference with the wrapped
source) through ordinary member destruction — with no guarantee the
wrapped source's in-flight asynchronous work has actually finished.

The wrapped source's own real implementations
(StreamingDataSource, PollingDataSource) have no custom,
blocking destructor
 — they use the compiler-generated default and
rely on weak_from_this()-guarded async callbacks that protect only
their own this pointer, not the externally-owned destination_/
status_manager_ raw pointer/reference they were handed. A callback
already in flight when Close() runs (e.g., an HTTP response or SSE
event arriving concurrently with an FDv2 recovery/shutdown) can
therefore land on freed memory.


Location

Element | File:Line -- | -- Interface contract being violated | libs/server-sdk/src/data_interfaces/source/idata_synchronizer.hpp:34 Violation — discarded completion callback | libs/server-sdk/src/data_systems/fdv2/fdv1_adapter_synchronizer.cpp:199 (Close(), defined at :191) Destroyed object #1 | ConvertingDestination, fdv1_adapter_synchronizer.hpp:92-104, owned via std::unique_ptr const destination_; Destroyed object #2 | DataSourceStatusManager status_manager_ — constructed by the adapter (fdv1_adapter_synchronizer.cpp:127) and passed by reference into the wrapped source's own constructor (:152, source_builder(*status_manager_)), so the wrapped source holds a reference into memory the adapter owns and will free Raw-pointer handoff | fdv1_source_->StartAsync(destination_.get(), /*bootstrap_data=*/nullptr) Destructor | ~FDv1AdapterSynchronizer(), fdv1_adapter_synchronizer.cpp:154 Real production destroy call site | FDv2DataSystem::OnFDv1RetryTimer(), fdv2_data_system.cpp:469 (also reachable via Close(), OnConditionFired, OnSynchronizerResult)

Activity

  1. TerryJLU commented on Aug 29, 2026

    @TerryJLU
    Author

    Proof of Concept
    Both PoCs use real, unmodified production code for every class
    involved (FDv1AdapterSynchronizer, ConvertingDestination,
    DataSourceStatusManager, and — in the second PoC — the real
    FDv2DataSystem orchestrator). The only test double, in both cases,
    is the wrapped IDataSynchronizer transport itself (the thing that
    would normally be a real StreamingDataSource/PollingDataSource
    backed by a real network connection) — this is the one extension
    point the SDK's own existing test suite (fdv1_adapter_synchronizer_test.cpp's
    MockFDv1Source) already fakes for this exact interface, so no new
    mocking approach was introduced.

    PoC 1 — direct, minimal repro (deterministic, no threading required)
    class DeferredShutdownFDv1Source final : public IDataSynchronizer {
    public:
    void StartAsync(IDestination* destination, ...) override {
    destination_ = destination;
    }
    void ShutdownAsync(std::function<void()> completion) override {
    deferred_completion_ = std::move(completion); // never invoked
    }
    IDestination* destination_ = nullptr;
    std::function<void()> deferred_completion_;
    };

    TEST(...) {
    IDestination* captured_destination = nullptr;
    {
    FDv1AdapterSynchronizer adapter(/* builder returning the mock above */);
    adapter.Next(data_model::Selector{}); // triggers StartAsync
    captured_destination = source->destination_;
    } // adapter destroyed: Close() -> ShutdownAsync([]{}) -> discarded
    // -> destination_ (ConvertingDestination) destroyed immediately

    // Simulates the wrapped source's own in-flight callback landing late.
    captured_destination->Upsert("late-update-key", std::move(descriptor));
    

    }
    ASan result (3/3 runs, identical clean heap-use-after-free each time):

    ERROR: AddressSanitizer: heap-use-after-free on address 0x503000017b90
    READ/WRITE at repro_fdv1_adapter_destination_uaf_test.cpp:157
    in TestBody() (the captured_destination->Upsert(...) call)

    freed by thread T0 here:
    #1 FDv1AdapterSynchronizer::ConvertingDestination::~ConvertingDestination()
    fdv1_adapter_synchronizer.hpp:92
    #4 FDv1AdapterSynchronizer::~FDv1AdapterSynchronizer()
    fdv1_adapter_synchronizer.cpp:154

    previously allocated by thread T0 here:
    #2 FDv1AdapterSynchronizer::FDv1AdapterSynchronizer(...)
    fdv1_adapter_synchronizer.cpp:126

    SUMMARY: AddressSanitizer: heap-use-after-free

  2. self-assigned this
    on Sep 1, 2026
  3. beekld commented on Sep 1, 2026

    @beekld
    Contributor

    I don't think this use-after-free is actually reachable from the public API, for two reasons:

    • The executor running this is single-threaded, so the calls can't be in flight at the same time.
    • The source is always destroyed before the destination, due to the declaration order.

    That said, it would be nice to clean this up, so that the internal code at least conforms its own documented contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingpackage: sdk/serverIssues affecting the C++ Server SDK

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions