fix: reassemble BLE prepared/long writes so iOS messages over 157 chars arrive (#90) - #739
Conversation
iOS sends packets larger than the negotiated ATT MTU using the BLE
prepared ("reliable"/long) write procedure: the payload arrives as
several onCharacteristicWriteRequest callbacks with preparedWrite=true
and increasing offsets, finalized by onExecuteWrite. The server only
handled the offset=0 chunk and tried to parse each chunk as a whole
packet, so anything over ~157 chars from iOS was silently dropped.
Buffer prepared-write chunks per device by offset and reassemble the
full payload in a new onExecuteWrite override before parsing. The
non-prepared write path is unchanged. The reassembly is factored into a
small, thread-safe GattPreparedWriteBuffer with a per-device size cap so
a broken or malicious peer cannot exhaust memory, and buffers are
cleared on execute, cancel, and disconnect.
Fixes permissionlesstech#90
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9de168d0d7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Log.i(TAG, "Server: Device disconnected ${device.address}") | ||
| connectionTracker.cleanupDeviceConnection(device.address) | ||
| // Drop any in-flight prepared-write buffer for this device to avoid leaks | ||
| preparedWriteBuffer.cancel(device.address) |
There was a problem hiding this comment.
When stop() sets isActive = false before cancelling connections, the resulting disconnect callback returns at the shutdown guard and never reaches this cancellation. Consequently, an in-flight prepared write survives a server stop/restart and may remain allocated or be executed after the same address reconnects. Clear all prepared-write buffers directly in stop(), or perform this per-device cleanup before the callback's isActive guard.
AGENTS.md reference: AGENTS.md:L63-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already handled: stop() calls preparedWriteBuffer.clearAll() before the isActive guard (commit e16ca70), so a server stop clears every in-flight prepared write regardless of the disconnect-callback timing. The normal disconnect path also cancels the per-device buffer directly at STATE_DISCONNECTED (not behind the guard), so a prepared write can't survive a stop/restart.
stop() marks the server inactive before connections are torn down, and the disconnect callback returns early once inactive, so it never reaches the per-device cancel path. Any half-finished prepared write would linger until the process ends. Clear all buffers directly in stop().
…te-reassembly # Conflicts: # app/src/main/java/com/bitchat/android/mesh/BluetoothGattServerManager.kt
Chessing234
left a comment
Chessing234
left a comment
There was a problem hiding this comment.
diagnosis and the buffer itself look right — offset-placed so out-of-order chunks are fine, overflow latched rather than allocating, cancel/disconnect/shutdown all clearing, and execute=false handled. two things around it.
append returning false is not surfaced to the peer. both rejection paths — negative offset, and a chunk that would pass maxPayloadSize — still answer GATT_SUCCESS with the echoed value, so the client sees every prepare accepted and only finds out at execute, where the payload is dropped with nothing sent back but success. ATT has codes for exactly these (GATT_INVALID_OFFSET, and prepare-queue-full/insufficient-resources for the cap), and a client that gets one can fall back or resend. as written a peer that trips the cap loses the message silently and has no way to know, which is the failure mode #90 was about in the first place.
second, a gap is indistinguishable from written zeros. chunks land in an array grown to endInt, so a client that writes offset 0 and offset 200 and nothing between reassembles as 200 bytes with a zero hole, and that goes to fromBinaryData as if it were complete. same for overlaps: a later chunk silently rewrites bytes an earlier one placed. mostly this just fails to parse, but the buffer can't tell "not yet written" from "wrote zeros", so it can't say which. tracking covered ranges and refusing an execute whose covered length ≠ length would close both. iOS is working the same ground in permissionlesstech/bitchat#1666 — worth comparing, though note their transport writes without response so their monotonic-offset rule doesn't transfer directly.
neither blocks the fix for #90; the reassembly path is the valuable part and it's sound.
Two robustness fixes from review: - Rejections now reach the peer. A negative offset and an over-cap chunk both answered GATT_SUCCESS, so the client saw every prepare accepted and only lost the payload at execute, silently — the failure mode permissionlesstech#90 was about. The server now returns GATT_INVALID_OFFSET for a negative offset and ATT "Prepare Queue Full" (0x09) when the chunk would exceed the buffer cap, so the peer can abort or resend. - A gap is no longer mistaken for zeros. Chunks land in an array grown to the highest offset, so an unwritten hole (write offset 0 then offset 200, nothing between) reassembled as zero-filled and parsed as complete. The buffer now tracks covered byte ranges and execute() refuses a payload that is not a single contiguous run over [0, length); overlaps are fine (last write wins). Tests: an unwritten gap yields null on execute, adjacent chunks that meet exactly reassemble, and an overlapping-but-contiguous write succeeds with the later bytes winning.
|
Both addressed — thanks, these are the right gaps to close.
Added tests for the gap (execute → null), adjacent chunks meeting exactly, and an overlapping-but-contiguous write. Good call on bitchat#1666 — I kept this to the offset/coverage checks rather than a monotonic-offset rule, since as you noted their without-response transport doesn't carry over here. No SDK on this machine so I couldn't run the suite locally, but the JUnit tests will run in CI. |
2 participants
What
Long messages from iOS (over ~157 chars) never showed up on Android. iOS sends anything larger than the negotiated ATT MTU as a BLE prepared ("reliable" / long) write: the payload arrives as several
onCharacteristicWriteRequestcallbacks withpreparedWrite=trueand increasing offsets, then a singleonExecuteWriteto finalize.The GATT server only ever looked at the
offset=0chunk and tried to parse each chunk as a completeBitchatPacket, so oversized packets failed to parse and were dropped. There was noonExecuteWriteoverride at all.Full credit to @hamzaozturk for the diagnosis in #90. Pinpointing
preparedWrite=truewith only the offset-0 chunk being processed is exactly what this fixes.Changes
GattPreparedWriteBuffer: thread-safe, per-device reassembly keyed by BLE address, chunks placed by offset, with a per-device size cap so a broken or malicious peer cannot exhaust memory.BluetoothGattServerManager: buffer prepared-write chunks (echoing offset and value in the response as the protocol requires) and add anonExecuteWriteoverride that reassembles the full payload before running the existing parse and dispatch path. Buffers are cleared on execute, cancel, and disconnect. The non-prepared write path is unchanged.Testing
./gradlew :app:testDebugUnitTestpasses: 130 tests, 0 failures, 11 new. The pure reassembly logic is fully covered. The GATT callback wiring itself needs a device or emulator, so the end-to-end prepared-write handshake was not exercised here; the change compiles and the buffer behaviour it feeds is tested.Fixes #90