Don't duplicate the complex preprocessor check multiple times, just
define wxHAS_TEST_IPC_SERVER once and check for it.
Also define the helper ShouldRunTestIPCServer() function which is more
clear than using wxGetEnv() explicitly.
No real changes.
Add a test exercising the IPC-over-sockets implementation from a single
thread and from multiple threads concurrently (Execute, Request, Poke,
Advise, combined Advise+Request, and concurrent main-thread and
worker-thread Request()s).
Each test starts its own server by re-executing the test program with
WX_IPC_TEST_SERVER set and shuts it down again in the fixture teardown, so
no server process outlives a test (or disturbs the unrelated GUI tests in
test_gui). The client runs in the main Catch2 process and queries the server
for state to verify it (Catch2 macros cannot run in the server process). The
wait loops are wall-clock bounded so they behave under a GUI event loop, and
a per-fixture watchdog aborts with a diagnostic if a test ever hangs rather
than letting CI time out.
The test runs in both the console "test" and the GUI "test_gui" programs. It
is excluded from one configuration: wxQt, whose event loop does not reliably
process a cross-thread CallAfter() (a wxQt bug fixed separately).
wxTCPClient::MakeConnection left the client socket at its long default
timeout, so connecting to a listener that is not yet ready to complete the
topic handshake blocked for that default (ten minutes) instead of failing
promptly for the caller to retry. Set the socket timeout to wxIPCTimeout
before connecting, matching the per-connection timeout used afterwards.
Also reword the reply-handoff comment to say the pending reply is serialized
by m_cs_awaiting_reply.
The IPC connection sockets are event-notified and wxSOCKET_WAITALL (not
wxSOCKET_BLOCK), so a main-thread Read/Peek that must wait pumps the event
loop. On wxMSW under a GUI event loop that pump re-dispatches a queued
wxSOCKET_INPUT for a socket we are already reading on the main thread, so
OnSocketInput() re-enters and reads the same socket again, tripping
wxSocketReadGuard ("read reentrancy?") and throwing out of the main loop.
This surfaced as the test_gui IPC test failing on wxMSW DLL Release x64;
the console and Linux/Xvfb GUI runs do not re-enter, so they passed. The
recursive m_cs_socket_processing critical section does not catch it.
Add a main-thread re-entrancy guard: track the sockets currently being read
(m_socketsBeingRead) and have OnSocketInput() bail when re-entered for a
socket already being read. The in-progress read consumes the data and the
outer loop / a re-posted wxSOCKET_INPUT drains the rest. SendAndGetReply_
MainThread() marks its socket the same way so a re-dispatched OnSocketInput()
cannot steal the reply. Reads run only on the main thread, so the set needs
no lock.
A Request() on the main thread concurrent with a worker-thread Request() on the
same connection deadlocked: the worker holds m_cs_awaiting_reply while parked in
RunOnMainThread() waiting for the main thread, and the main thread blocked
acquiring m_cs_awaiting_reply in SendAndGetReply_MainThread(), so it stopped
pumping the event loop the worker needed, and neither could progress.
Fix: the main-thread path no longer blocks on m_cs_awaiting_reply. It spins on
TryEnter(), and between attempts pumps the active event loop
(ProcessPendingEvents() to run the worker's RunOnMainThread() jobs, plus a short
DispatchTimeout() to service its reply). The worker can then finish and release
the lock while the main thread keeps the loop alive. The pump happens before
either lock is taken, so re-entrant OnSocketInput() cannot collide with a lock
we hold; the existing read loop after acquisition is unchanged. A CritSectLeaver
RAII guard releases the manually-entered section on all return paths. If there
is no active event loop (so nothing could advance a worker), it falls back to a
plain blocking Enter().
With this, a main-thread Request() concurrent with a worker-thread Request()
completes instead of hanging. The IPC::ConcurrentMainAndWorkerRequest test added
with the test suite that follows reproduces the original deadlock; with the fix
in place, full [ipc] is green on a plain build and under TSan (no data races,
no hang).
wxSocket's event-notification machinery is effectively main-thread-only:
the process-global wxFDIODispatcher's handler map is mutated whenever a
socket re-arms input notification during I/O. Worker threads calling
Request()/Advise()/etc. did that I/O themselves, racing the main event
loop's epoll_wait/dispatch over the dispatcher map, the data race
behind the macOS AdviseAndRequestMultiThread failure (and the Linux
"modify descriptor -1 in epoll" errors).
Route all IPC socket I/O through the main thread: RunOnMainThread() runs
the work directly on the main thread, else marshals it via CallAfter()
and blocks the worker on a semaphore until it completes. The six
worker-callable transaction methods funnel their socket work through it.
Making that path correct under multithreaded use also required:
- ReadMessageFromSocket(): bind the wxIPCMessageNull to the socket before
ReadIPCCode(), otherwise every connect handshake read an empty message
and the server refused all connections.
- SendAndGetReply_WorkerThread(): obey the wxCondition contract: hold
m_replyMutex around the WaitTimeout() (predicate-guarded loop), but NOT
across WriteMessageToSocket(). Holding it across the marshalled write
deadlocked the worker against the main thread's
OnSocketInput()->DeliverPendingReply().
- OnSocketInput(): test the pending-reply match inside DeliverPendingReply()
under m_replyMutex instead of reading m_pending unlocked (TSan data race).
- Move gs_critical_io acquisition inside the RunOnMainThread() lambda so the
socket-I/O lock is only ever held on the thread doing the I/O, never across
the worker->main handoff.
Reply-expecting commands are serialized via m_cs_awaiting_reply, taken in
both the main- and worker-thread paths. A remaining deadlock between a
main-thread and a worker-thread Request() on the same connection is
addressed in the following commit.
A shared, process-lifetime handler can receive a socket event queued
before its socket was destroyed. Client_OnRequest obtained the socket via
event.GetSocket(), a checked downcast (wxObject* -> wxSocketBase*) that is
undefined behavior on a freed object and trips UBSAN's vptr check, before
the IsConnectionSocket() registry guard could reject it. Compute the
pointer with reinterpret_cast (not vptr-instrumented) purely for the
registry lookup; the object is only dereferenced once the guard confirms
it is still alive.
FindMessage() may run on a worker thread (Request()/Advise()) off the
main event loop. When it consumes a message, the socket's own
wxSOCKET_INPUT notification for trailing data can already have been taken
and never reach the main loop, so a following message (e.g. an Advise) is
never drained, the lost-wakeup behind the macOS
AdviseAndRequestMultiThread failure. Re-post wxSOCKET_INPUT
unconditionally so the main loop always re-scans the socket; a spurious
event is harmless (the handler peeks, finds nothing, returns).
wxTCPEventHandler is a process-wide singleton shared by every IPC
connection and it lives until the program exits. Socket events queued on
it can still be pending after the wxSocketBase they refer to has been
destroyed (e.g. a wxSOCKET_LOST generated while a connection is torn
down). Because wxSocketBase::IsOk() only checks m_impl != nullptr, a
freed socket with a dangling m_impl passes that check and
Client_OnRequest() goes on to call socket->Peek(), dereferencing freed
memory.
This use-after-free crashed reliably under the MSVC debug heap (freed
memory poisoned to 0xFEEEFEEE) and was otherwise silent, manifesting as
an intermittent SIGSEGV in the IPC unit tests on Windows when a stale
event left by one test case was dispatched during the next test's setup.
Track the set of sockets that currently have a live connection in the
handler and ignore socket events for any socket not in that set. The
check only compares pointer values and never dereferences a
possibly-freed socket. Sockets are registered when a connection is
established (client MakeConnection and server Server_OnRequest) and
unregistered when it is torn down (HandleDisconnect and
~wxTCPConnection).
Serialize all wxIPC socket reads and writes under one critical section. Separate read/write locks allowed concurrent access to wxSocketBase from worker and main threads.
FD_CONNECT, FD_WRITE, FD_READ are non-exclusive notifications in
theory, ie there is a notification with an or'd FD_CONNECT|FD_READ. By
throwing this away, there are occasional stoppages in the data flow,
particularly right after connect.
In any case, I don't understand why sockmsw was throwing any
notification away. It should be up to the callback whether or not
action is to be taken on a notification. As such, I've eliminated
the filter.
On windows, reading or peeking when there is no data waiting results
in WSAECONNABORTED and ERROR_ACCESS_DENIED. Since there is no other
way to determine whether or not we've read all information out of the
buffer, make this error non-fatal.
There is a critical section to ensure that there are no overlapping
IPC_REQUESTs. This ensures that there is only one possible
IPC_REQUEST_REPLY for each request.
One thing in the previous version: Multiple threads requesting
information would run into problems if data reads overlapped, perhaps
in multiple threads.
The source of this problem is that the memory management from data
reads in wxConnection is not transferred to to the caller. So
wxConnection uses a buffer that gets overwritten for every read.
My approach is to create an array of char* pointers: Every memory
allocation is stored in the array, and survives until another
MAX_MSG_BUFFERS is allocated, at which point the previous data is
deleted and a new allocation occurs.
The deletion of previous data is still potentially problematic (the
caller needs to understand that passed data must be copied in a
reasonable amount of time if they plan to keep it around), but at
least the data will have a lifetime greater than one read.
I set the array to 2048 blocks, which was more for my app than
anything, but maybe there's a better value.
As an example, IPC_REQUEST from the client expects the specific reply
of IPC_REQUEST_REPLY. It is possible that the server sneaks in other
messages, such as a stream of IPC_ADVISE data. So FindMessage loops
until the desired message is found, executing any other unexpected
messages that happen to be read first.
- There is no guarentee that a notification from the socket
corresponds to a single wxIPCMessage transmission. So Client_OnRequest
loops in Read/Execute until Peek reveals that there are no further
messages waiting.
Each IPC Message needs to have
- constructor for reading
- constructor for writing (usually different from read)
- DataToSocket() method for writing
- DataFromSocket() method for reading
The DataToSocket() and DataFromSocket() must be symmetric.
- The IPCCode is expanded from 8 bits to 32 bits. The first 24 bits
serve as a constant header to ensure that data sync is not lost.
Before this change, loss of sync meant there was an 11 out of 256
chance that an incoming byte would be interpreted as a command,
possibly leading to a crash. The header reduces the probability by a
factor of 2^24.
- Each derived message (Request, Execute, etc) will have a
DataToSocket() and DataFromSocket() method. These methods allow for
easy verification that the reads and writes are correctly symmetric.
- Pointers to Read and write data are separate member vars, for easier
management of const-params for the wxConnection interface (which we
take to be immutable).
- A pointer back to wxTCPEventHandler is going to be needed for
memory management. More on this in a later commit.
Note that source archives checksums have surreptitiously changed since
the initial announcement as the archives had to be rebuilt. The ones
here are the correct ones.
This is needed for translucency effects, desktop background images
changing the toplevel window background tint.
Also don't erase background when no background color is specified.
Closes#26301.
Vadim Zeitlin
John Paul Mattia
Stefan Csomor