Skip to content

fix: [viewer] make RemoteServer message handling connection-safe - #10308

Open
carlos-al wants to merge 1 commit into
google:mainfrom
carlos-al:fix/fil005-remoteserver-races
Open

fix: [viewer] make RemoteServer message handling connection-safe#10308
carlos-al wants to merge 1 commit into
google:mainfrom
carlos-al:fix/fil005-remoteserver-races

Conversation

@carlos-al

Copy link
Copy Markdown

Summary

This PR fixes two memory-safety issues in RemoteServer message handling:

  • Concurrent WebSocket connections shared one fragment-assembly buffer.
  • Android acquired a message through separate length, label, and copy operations.

The first allowed callbacks from different CivetWeb workers to invalidate shared vector storage. The second allowed the queued message to change after Java selected its allocation and copy length.

Analysis

Concurrent WebSocket assembly

MessageReceiver stored fragment state in a single mChunk vector and mReceivedMessage pointer.

CivetWeb can invoke the same receiver concurrently for different mg_connection objects. Interleaved fragmented messages could therefore splice state from unrelated clients or reallocate shared vector storage while another worker was
using it.

The missing-defense regression reproduces this as an ASan memory error across two CivetWeb workers.

Android message acquisition

Android previously:

  1. Read the queued message length.
  2. Read its label.
  3. Allocated a ByteBuffer.
  4. Asked JNI to pop and copy the message using the earlier length.

A same-label replacement between those operations could make JNI copy the stale length from a different, shorter message.

The Fix

Track fragment assembly independently for each mg_connection and serialize access to that state.

Closing a connection removes only its partial message, and server teardown drains remaining partial state after CivetWeb workers stop.

Replace Android’s multi-call protocol with one JNI acquisition operation. JNI now pops one message, validates its actual size, allocates a matching direct buffer, copies exactly that message, and assigns its label and buffer to the
same Java result object.

The public Java acquireReceivedMessage() API remains unchanged.

Testing

Added six loopback WebSocket regressions covering:

  • Interleaved fragmented messages from two simultaneous connections
  • Disconnect cleanup for an incomplete message
  • Same-label replacement
  • Zero-length messages
  • Empty-queue behavior
  • Server destruction with connected partial messages

Additional verification:

  • Missing-defense build reproduced the memory error under ASan
  • Patched ASan+UBSan regression suite: 6 tests passed
  • Existing viewer settings suite: 6 tests passed
  • Original two-client 1 MiB message scenario completed without sanitizer findings
  • Built the Android arm64 viewer and complete JNI library
  • Packaged and installed the patched sample APK
  • Exercised replacement and control scenarios on a physical Samsung arm64 device without a native or Java crash

@pixelflinger pixelflinger added the internal Issue/PR does not affect clients label Aug 14, 2026
}

char const * RemoteServer::peekIncomingLabel() const {
static thread_local std::string label;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this static thread_local causes another problem, which is the returned value could be changed as the same thread calls peekIncomingLabel() again when a new message just arrives.

I see this function is used by one thread at this moment, so technically the current impl might be safe for now. But it's not recommended to use thread_local to just to store data. I would make it return std::string or make this function to take a caller-supplied buffer.

return true;
}

std::lock_guard lock(mConnectionsMutex);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this lock looks to serialize too many things (insert, new, memcpy,e tc), which seems unnecessary. could you scope this to just mConnections then process the rest outside of it?

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

Labels

internal Issue/PR does not affect clients

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants