- dispatcher tests: register the server before the client connects
(Connect race), poll the Connected ack non-blocking with a deadline,
keep the client socket non-blocking so a full buffer yields EAGAIN
instead of a blocking send() past any deadline
- piethernet test: use a TCP client against a never-reading listener
(server-object writeDevice is a no-op) and 32 MB to exceed the
~10 MB loopback buffers
With piethernet/pistreampacker reverted to master all four tests
fail (8 ms .. 40 s); with the fixes ctest is 342/342.
- PICloudDispatcher.ServerDisconnectWhileClientForwarding: a disconnecting
logical server must not freeze the dispatcher while a client's read
thread is jammed forwarding data to an unread server socket
- PICloudDispatcher.ClientRemovableWhileServerNotReading: the client's
own Disconnect frame must be processed and the client removed even
though the stall happens in the other connection's writeDevice()
- PIEthernet.WriteThreadStopsWhilePeerNotReading: the EAGAIN retry loop
in writeDevice() must not prevent stopAndWait() from returning
- PIStreamPacker.RejectsOversizedPacket: a forged INT_MAX packet size
from the stream must not be accumulated, and the packer must stay
usable for valid packets
While the peer does not drain its receive buffer, every forwarded packet
would re-stall the reader thread for up to writeTimeout(), so a queued
Disconnect frame could wait behind the whole backlog. Remember a stalled
target (send slower than 2 s) and drop data for a short cooldown instead
of stalling; keep dispatcher writes bounded at 3 s. Control frames
(Connected/Disconnected) are unaffected.
The loop may run in a thread that belongs to another device (the picloud
dispatcher forwards data from one connection's read thread to another
connection's socket), so isThreadedReadStopping() alone cannot stop it:
it sees the wrong device's flag. Stalling for at most writeTimeout()
without progress makes every write finite; the receiver resynchronizes
on the next packet signature.
newConnection pushed the client into clients while holding map_mutex
but started its read thread after releasing it; in that window the
client was a live member of clients with no read thread and could be
marked, closed and deleted by the drainer before start() ran. Start
the thread inside the locked section: the thread can only wait for
map_mutex after the function has released it, so this is not a
deadlock, and the 'everything in clients is started' invariant holds.
~DispatcherServer dropped clients/servers pending in rmrf_clients/
rmrf_servers without closing or deleting them (object leak of live
PIObjects). Collect them together with the main maps, then run the
same stop/close/delete sequence. The sets are disjoint with the main
maps by construction (items are removed from the main maps when
marked), so there are no duplicates.
The dataReadedServer lambda looked up the sub-client under
mutex_clients but called sendData() after the unlock; the client could
be deleted by the drainer in between (same UAF pattern that was fixed
in PICloudServer::_readed). Keep the send inside the locked section:
sendData only takes mutex_send/prog_s_mutex downstream, so the lock
hold is bounded.
cleanClients() (timer thread) called close()/stopAndWait() and
disconnectClient() (read thread) did the same while holding
map_mutex, while client read threads wait for the same lock in
registerServer/registerClient/disconnectClient — a guaranteed
deadlock (the -w watchdog aborts exactly this). disconnectClient
could also wait for its own read thread (self-wait in
waitThreadedReadFinished) and the destructor never stopped the timer
or drained the clients, leaving dangling lambda captures of this.
Restructure around a single drainer (the timer thread):
- cleanClients: under map_mutex only mark (move rmrf_* out, mark
ping-timeout/orphaned clients); after the unlock close
(stopAndWait) and delete the connections, then delete the servers
- disconnectClient: only mark under the lock, then request a
non-blocking stop() on the affected connections; the drainer waits
and deletes (no self-wait, nothing blocks while holding the lock)
- newConnection: start the read thread after releasing the lock
- ~DispatcherServer: stop watchdog/timer first, then close and delete
all clients before the servers (raw-pointer order) and eth
The watchdog is now a PIThread member so the destructor can stop it;
setMaxConnections takes map_mutex like its readers.
stop() iterated clients while addClient()/removeClient() modified it
from other threads (reallocated vector iterators, data race). Copy the
list under mutex_clients and close the copy.
last_ping (PITimeMeasurer) was reset from the read thread
(dataReadedServer/pingReceived) without any lock while cleanClients
read it via lastPing() in the timer thread. Reset and read it under
mutex_clients (lock order map_mutex -> mutex_clients, consistent with
the registerClient path).
PICloudClient::internalDisconnect() cleared buff without mutex_buff
while the eth read thread appends to it concurrently (close() races
with in-flight Data frames), and bytesAvailable() read buff.size()
lock-free on both PICloudClient and PICloudServer::Client. Take the
lock in both places (mutex_buff made mutable for the const getter).
In the Data case the client pointer was looked up under clients_mutex
but used (pushBuffer) after the unlock; a user-initiated delete() of
the same client raced with the read thread and hit freed memory.
In the Disconnect case the client was stopped/closed and appended to
removed_clients_ (without the lock, unlike every other site) after
the unlock, with the same UAF window and a race on the vector itself.
Do both uses while holding clients_mutex: a concurrent delete() is
serialized with this block by clientDeleted() (the deleted() event),
so the client cannot be freed while it is in use. Setting
is_connected=false first also stops pushBuffer from buffering into a
dying client.
sendStart/sendConnected/sendDisconnected/sendData/sendPing mutated
the shared header.type and serialized the header without holding
mutex_send (it only covered streampacker->send), so concurrent
senders (e.g. ping timer vs app thread on PICloudServer) could mix
type/role bytes of different frames. Snapshot the header into a
local copy under mutex_send in every send* and setRole now takes the
lock as well.
parseDataServer/parseConnect/parseDisconnect read a uint without
checking the payload size; PIByteArray does a partial memcpy on
undersized input, so a short frame produced a garbage client id.
Return the default value when the payload is shorter than sizeof(uint).
prog_s_mutex/prog_r_mutex were declared but never locked, so
concurrent send()/received()/clear()/receivePacketProgress() raced on
shared buffers and fragments of different senders interleaved on the
wire. Lock the receive state (stream/packet) with prog_r_mutex in
received()/clear()/receivePacketProgress() and the whole emission of
one packet with prog_s_mutex in send().
Additionally the size field of a packet taken from the stream was
only checked for negativity: a remote peer could declare INT_MAX and
the packer accumulated memory unboundedly, swallowing all following
data as phantom payload (permanent desync, no resync). Add a
max_payload_size cap (config, default 64MB) and resynchronize byte by
byte when the declared size exceeds it, so valid packets after the
corrupted one survive.
Also drop frames whose size does not fit the 32-bit wire format
instead of wrapping the length field.
Socket is non-blocking while BlockingWrite is set for every PIEthernet
(construct()), so a stalled peer made ::send return EAGAIN and the
retry loop spun forever. The thread could no longer be stopped by
stop()/stopAndWait() (interrupt only wakes event waits), which turned
into a permanent hang for PIStreamPacker-based users such as the
picloud dispatcher. Break out of the loop when the device is
stopping and return -1.
1. Missing break in readDevice() Bulk case: Bulk read result was
overwritten by interrupt read, silently corrupting all bulk reads.
2. Wrong variable in writeDevice(): switch checked ep_read instead
of ep_write; Interrupt case used ep_read.address for writes.
3. Interface mismatch in closeDevice(): always released interface 0
(intefrace_) instead of the actually claimed interface
(interface_claimed), leaking non-zero interface claims.
4. Wrong index in interface enumeration: used outer loop variable c
(configuration index) instead of i (interface index), reading
wrong interface descriptors or causing OOB access.
destroy() aborted and deleted clients in the queue but did not
delete clients that were currently being processed (clients_in_proc).
If a thread was terminated while processing a client, that client
object and its CURL handle were leaked.
If clCreateContext succeeded but clCreateCommandQueue failed,
the context handle was returned without being released, leaking
the OpenCL context and all associated GPU resources.
luaL_newstate() allocates a Lua state (~2-4 MB with libraries)
in the constructor, but there was no destructor to call lua_close().
Every PILuaProgram instance leaked its entire Lua state.
Add ~PILuaProgram() that calls lua_close(PRIVATE->lua_state).
The decrypt() function for crypto_box (public-key) used
crypto_secretbox_NONCEBYTES and crypto_secretbox_MACBYTES
(secret-key constants) instead of crypto_box_* constants.
These happen to be equal in libsodium but are semantically
different APIs. Fix ensures consistency with the encrypt()
counterpart.
piDecompress() appended original size as 8-byte ullong at the end
of compressed data, but passed zba.size() (including the 8 bytes)
to uncompress(). This fed trailing metadata as part of the zlib
stream. Fix: subtract sizeof(ullong) from source length.
received() acquired mutex_header and mutex_session in different orders
depending on packet type:
- pt_Data path: mutex_header → mutex_session
- pt_Start path: mutex_session → mutex_header
send_process() acquires mutex_session independently. When running
concurrently with received(), the reversed lock ordering creates a
classic ABBA deadlock scenario.
Fix: enforce consistent ordering (mutex_header → mutex_session) in
all code paths. Restructured pt_Start case and buildSession() to
always acquire mutex_header before mutex_session.
The background run() thread copies allConnections() then iterates
each connection's diags_ map. If a PIConnection is deleted from
another thread between the copy and the iteration, dereferencing
the pointer is UAF. Add null check and protect diags_ iteration
with __device_pool__ lock.
Flags break_, is_sending, is_receiving, is_pause are accessed from
both the send thread (send_process) and the read thread (received)
without synchronization. Plain bool reads/writes from multiple threads
is undefined behavior per C++ standard. Convert to std::atomic<bool>
with proper initializers.
piMini(max_size, buff.size()) implicitly narrows size_t to int,
which is UB for large buffers. Replace with explicit piMin<ssize_t>
matching the correct pattern used in picloudclient.cpp.
The buffer was sized as floor(sz/4)*3 but the loop ran ceil(sz/4)
times, writing 3 bytes per iteration. For sz%4!=0, this wrote past
the buffer end. Also guarded sz<4 to avoid processing trivially
short or malformed input.
The function called unlock() on thread_mutex without a matching
lock(), causing UB on Windows (releasing an unowned critical
section) or silently dropping a user-held lock.
When elapsed time exceeded the timeout, the remaining milliseconds
expression went negative and was implicitly converted to DWORD,
wrapping to ~0xFFFFFFFF (5 days) — effectively INFINITE.
Now clamps to 0 and returns false when time has elapsed.
dirent** list was uninitialized, so scandir failure (returning -1)
left it with indeterminate value. The unconditional free(list) at
the end was then UB — typically heap corruption or crash.
pop_back() called resize(size_ - 1) without checking for empty.
On empty array, uint underflow produced UINT_MAX, causing
bytesInBits(UINT_MAX) overflow and subsequent OOB access.
pop_front() already had this guard; pop_back() was missing it.
Three complex type cases (pivComplexf, pivComplexd, pivComplexld)
lacked break statements, causing fallthrough to pivTime. Setting
a complex variant from string silently overwrote the value with
PITime::fromString(), corrupting the variant's type and data.
When mmap() returned MAP_FAILED, data stayed as (void*)-1, causing
subsequent read/write/close to pass the null guard and crash.
Now resets data=nullptr, and calls shm_unlink when owner=true to
avoid orphaned shared memory objects.
The float FFT plan generator wrote 8 ints per entry (indices +0..+7)
but declared entrysize=4, causing heap overwrite into the next entry
and incorrect plan array sizing.
andrey
peri4