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.
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.
processFile() accessed files_[id - 1] without validating id.
When id == 0 (from a crafted network packet), id - 1 = -1 caused
out-of-bounds access. Added bounds check: id must be in range
[1, files_.size()]. Stops receive and logs error on invalid id.
fork() can return -1 on error (too many processes, out of memory).
The old code treated pid_ == -1 as a successful fork, entering the
parent branch and calling waitpid(-1, ...) which waits for ANY
child process. Added explicit check: close pipes, free memory,
and return on fork failure.
In the blocking read path (timeout_ms <= 0), the while loop had no
exit condition for readDevice() returning 0 (EOF) or -1 (error).
This caused an infinite busy-loop at 100% CPU. Added 'else break'
to exit the loop on read failure, matching the timeout branch
which already handles this case.
readDevice() did pos += size instead of pos += ret. The clamping
on the next line (pos = min(pos, data_->size_s())) masked the
issue in practice, but the semantics were wrong. Aligns with
writeDevice() which correctly uses pos += rs.size_s().
open() returns -1 on error. The check fd != 0 treated -1 as a
valid fd (since -1 != 0 is true), causing devctl(-1, ...) and
close(-1) on QNX. Fixed to fd >= 0.
andrey
peri4