Fix race condition in SDK IO callback (#40664)
* Fix race condition in SDK IO callback * Apply PR feedback * Format * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Blue committed
May 29, 2026 at 10:49 UTC
b4d745717a5c3eddfb29bb83d564e70d29fb8d36
4 files changed
+144
-6
src/windows/WslcSDK/IOCallback.cpp
+21
-4
@@ -79,19 +79,36 @@ IOCallback::IOCallback(IWSLCProcess* process, const WslcContainerProcessIOCallba
79
}
80
81
IOCallback::~IOCallback()
82
+try
83
{
84
Cancel();
84
- if (m_thread.joinable())
85
- {
86
- m_thread.join();
87
- }
85
+ Complete();
86
}
87
+CATCH_LOG();
88
89
void IOCallback::Cancel()
90
{
91
m_cancelEvent.SetEvent();
92
}
93
94
+void IOCallback::Complete()
95
+{
96
+ // Complete can be called by multiple threads. Make sure that it's only ever called once since join() is not thread safe.
97
+ std::call_once(m_join, [this]() {
98
+ if (m_thread.joinable())
99
+ {
100
+ THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_HANDLE_STATE), IsOnIOCallbackThread());
101
+
102
+ m_thread.join();
103
+ }
104
+ });
105
+}
106
+
107
+bool IOCallback::IsOnIOCallbackThread() const noexcept
108
+{
109
+ return m_thread.get_id() == std::this_thread::get_id();
110
+}
111
+
112
bool IOCallback::HasIOCallback(const WslcContainerProcessOptionsInternal* options)
113
{
114
return options && HasIOCallback(options->ioCallbacks);
src/windows/WslcSDK/IOCallback.h
+4
@@ -25,6 +25,9 @@ struct IOCallback
25
~IOCallback();
26
27
void Cancel();
28
+ void Complete();
29
+
30
+ bool IsOnIOCallbackThread() const noexcept;
31
32
static bool HasIOCallback(const WslcContainerProcessOptionsInternal* options);
33
static bool HasIOCallback(const WslcContainerProcessIOCallbackOptions& options);
@@ -37,4 +40,5 @@ private:
40
std::thread m_thread;
41
wsl::windows::common::io::MultiHandleWait m_io;
42
wil::unique_event m_cancelEvent{wil::EventOptions::ManualReset};
43
+ std::once_flag m_join;
44
};
src/windows/WslcSDK/wslcsdk.cpp
+52
-2
@@ -616,7 +616,30 @@ CATCH_RETURN();
616
STDAPI WslcReleaseContainer(_In_ WslcContainer container)
617
try
618
{
619
- CheckAndGetInternalTypeUniquePointer(container);
619
+ // Reject release attempts originating from the container's own IO thread.
620
+ {
621
+ auto* peek = CheckAndGetInternalType(container);
622
+ auto ioCallback = peek->ioCallbacks.load();
623
+ RETURN_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_HANDLE_STATE), ioCallback && ioCallback->IsOnIOCallbackThread());
624
+ }
625
+
626
+ auto internalType = CheckAndGetInternalTypeUniquePointer(container);
627
+ auto ioCallback = internalType->ioCallbacks.load();
628
+ if (ioCallback)
629
+ {
630
+ // If the container has an IO callback registered, and the container has exited, wait until the IO callback has processed all IO.
631
+ try
632
+ {
633
+ WSLCContainerState state{};
634
+ THROW_IF_FAILED(internalType->container->GetState(&state));
635
+
636
+ if (state == WslcContainerStateExited || state == WslcContainerStateDeleted)
637
+ {
638
+ ioCallback->Complete();
639
+ }
640
+ }
641
+ CATCH_LOG();
642
+ }
643
644
return S_OK;
645
}
@@ -625,7 +648,34 @@ CATCH_RETURN();
648
STDAPI WslcReleaseProcess(_In_ WslcProcess process)
649
try
650
{
628
- CheckAndGetInternalTypeUniquePointer(process);
651
+ // Reject release attempts originating from the process's own IO thread.
652
+ {
653
+ auto* peek = CheckAndGetInternalType(process);
654
+ if (peek->ioCallbacks && peek->ioCallbacks->IsOnIOCallbackThread())
655
+ {
656
+ RETURN_HR(HRESULT_FROM_WIN32(ERROR_INVALID_HANDLE_STATE));
657
+ }
658
+ }
659
+
660
+ auto internalType = CheckAndGetInternalTypeUniquePointer(process);
661
+ if (internalType->ioCallbacks)
662
+ {
663
+ // If the process has an IO callback registered, and the process is exited, wait until the IO callback has processed all IO.
664
+ // If the process is released while still running, cancel the IO callback so we don't get stuck since the process might still be emitting IO.
665
+
666
+ try
667
+ {
668
+ WSLCProcessState state{};
669
+ int exitCode{};
670
+ THROW_IF_FAILED(internalType->process->GetState(&state, &exitCode));
671
+
672
+ if (state == WslcProcessStateExited || state == WslcProcessStateSignalled)
673
+ {
674
+ internalType->ioCallbacks->Complete();
675
+ }
676
+ }
677
+ CATCH_LOG();
678
+ }
679
680
return S_OK;
681
}
test/windows/WslcSdkTests.cpp
+67
@@ -1870,6 +1870,73 @@ class WslcSdkTests
1870
VERIFY_ARE_EQUAL(stdoutData.size(), c_expectedBytes);
1871
}
1872
1873
+ WSLC_TEST_METHOD(ReleaseFromIOCallbackFails)
1874
+ {
1875
+ struct Context
1876
+ {
1877
+ std::atomic<WslcProcess> process{nullptr};
1878
+ std::atomic<WslcContainer> container{nullptr};
1879
+ std::atomic<HRESULT> releaseProcessHr{S_OK};
1880
+ std::atomic<HRESULT> releaseContainerHr{S_OK};
1881
+ std::atomic<bool> captured{false};
1882
+ wil::unique_event done{wil::EventOptions::ManualReset};
1883
+ } ctx;
1884
+
1885
+ auto ioCb = [](WslcProcessIOHandle, const BYTE*, uint32_t, PVOID c) {
1886
+ auto* cx = static_cast<Context*>(c);
1887
+
1888
+ // Wait until the test thread has published both handles before sampling.
1889
+ auto process = cx->process.load(std::memory_order_acquire);
1890
+ auto container = cx->container.load(std::memory_order_acquire);
1891
+ if (!process || !container)
1892
+ {
1893
+ return;
1894
+ }
1895
+
1896
+ // Only capture on the first eligible callback; later callbacks no-op.
1897
+ bool expected = false;
1898
+ if (!cx->captured.compare_exchange_strong(expected, true))
1899
+ {
1900
+ return;
1901
+ }
1902
+
1903
+ // Both calls should fail with ERROR_INVALID_HANDLE_STATE without consuming the handles.
1904
+ cx->releaseProcessHr.store(WslcReleaseProcess(process));
1905
+ cx->releaseContainerHr.store(WslcReleaseContainer(container));
1906
+ cx->done.SetEvent();
1907
+ };
1908
+
1909
+ // Continuous writer for the init process so onStdOut fires repeatedly.
1910
+ WslcProcessSettings procSettings;
1911
+ VERIFY_SUCCEEDED(WslcInitProcessSettings(&procSettings));
1912
+ const char* argv[] = {"/bin/sh", "-c", "while true; do echo LINE; sleep 0.05; done"};
1913
+ VERIFY_SUCCEEDED(WslcSetProcessSettingsCmdLine(&procSettings, argv, ARRAYSIZE(argv)));
1914
+
1915
+ WslcProcessCallbacks callbacks{};
1916
+ callbacks.onStdOut = ioCb;
1917
+ VERIFY_SUCCEEDED(WslcSetProcessSettingsCallbacks(&procSettings, &callbacks, &ctx));
1918
+
1919
+ WslcContainerSettings containerSettings;
1920
+ VERIFY_SUCCEEDED(WslcInitContainerSettings("debian:latest", &containerSettings));
1921
+ VERIFY_SUCCEEDED(WslcSetContainerSettingsInitProcess(&containerSettings, &procSettings));
1922
+
1923
+ UniqueContainer container;
1924
+ VERIFY_SUCCEEDED(WslcCreateContainer(m_defaultSession, &containerSettings, &container, nullptr));
1925
+ VERIFY_SUCCEEDED(WslcStartContainer(container.get(), WSLC_CONTAINER_START_FLAG_ATTACH, nullptr));
1926
+
1927
+ UniqueProcess process;
1928
+ VERIFY_SUCCEEDED(WslcGetContainerInitProcess(container.get(), &process));
1929
+
1930
+ // Publish handles to the callback now that both are valid.
1931
+ ctx.container.store(container.get(), std::memory_order_release);
1932
+ ctx.process.store(process.get(), std::memory_order_release);
1933
+
1934
+ VERIFY_ARE_EQUAL(WaitForSingleObject(ctx.done.get(), 30 * 1000), static_cast<DWORD>(WAIT_OBJECT_0));
1935
+
1936
+ VERIFY_ARE_EQUAL(ctx.releaseProcessHr.load(), HRESULT_FROM_WIN32(ERROR_INVALID_HANDLE_STATE));
1937
+ VERIFY_ARE_EQUAL(ctx.releaseContainerHr.load(), HRESULT_FROM_WIN32(ERROR_INVALID_HANDLE_STATE));
1938
+ }
1939
+
1940
// -----------------------------------------------------------------------
1941
// Storage tests
1942
// -----------------------------------------------------------------------