-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: reassemble BLE prepared/long writes so iOS messages over 157 chars arrive (#90) #739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
gunjanjaswal
wants to merge
4
commits into
permissionlesstech:main
Choose a base branch
from
gunjanjaswal:fix/90-ble-long-write-reassembly
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
9de168d
fix(mesh): reassemble BLE prepared/long writes on the GATT server
gunjanjaswal e16ca70
Clear prepared-write buffers when the GATT server stops
gunjanjaswal 7ff6256
Merge remote-tracking branch 'upstream/main' into fix/90-ble-long-wri…
gunjanjaswal fb2bb0a
Surface prepared-write rejections and reject gapped payloads
gunjanjaswal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
145 changes: 145 additions & 0 deletions
145
app/src/main/java/com/bitchat/android/mesh/GattPreparedWriteBuffer.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| package com.bitchat.android.mesh | ||
|
|
||
| import java.util.concurrent.ConcurrentHashMap | ||
|
|
||
| /** | ||
| * Reassembles BLE prepared ("reliable" / long) writes on the GATT server side. | ||
| * | ||
| * When a peer (notably iOS) needs to send a payload larger than the negotiated | ||
| * ATT MTU, it uses the ATT prepared-write procedure: the payload arrives as a | ||
| * series of `onCharacteristicWriteRequest` callbacks with `preparedWrite == true` | ||
| * and increasing `offset`s, and is finalized by a single `onExecuteWrite`. Each | ||
| * individual chunk is only a slice of the packet, so parsing a chunk on its own | ||
| * fails. This buffer collects the chunks per device and returns the concatenated | ||
| * payload once the write is executed. | ||
| * | ||
| * All operations are thread-safe: these GATT callbacks can arrive on binder | ||
| * threads and several devices may be writing concurrently. Each device is keyed | ||
| * by a stable string (its BLE address) so device buffers are isolated. | ||
| * | ||
| * A per-device size cap protects against a broken or malicious peer streaming an | ||
| * unbounded prepared write to exhaust memory; once exceeded the buffer is dropped | ||
| * and [execute] returns null. | ||
| */ | ||
| class GattPreparedWriteBuffer( | ||
| private val maxPayloadSize: Int = DEFAULT_MAX_PAYLOAD_SIZE | ||
| ) { | ||
| companion object { | ||
| /** | ||
| * Generous upper bound for a single reassembled payload. Real bitchat | ||
| * packets are far smaller (well under the low-KB range), so this only | ||
| * ever trips for abusive peers. | ||
| */ | ||
| const val DEFAULT_MAX_PAYLOAD_SIZE = 512 * 1024 // 512 KiB | ||
| } | ||
|
|
||
| private class DeviceBuffer { | ||
| var data: ByteArray = ByteArray(0) | ||
| var length: Int = 0 | ||
| var overflowed: Boolean = false | ||
| // Byte ranges [start, end) actually written, kept sorted and merged. `data` | ||
| // is grown to the highest offset seen, so without this an unwritten gap | ||
| // would reassemble as zero-filled and parse as a complete packet. Tracking | ||
| // coverage lets execute() tell "not yet written" from "wrote zeros". | ||
| val covered: MutableList<IntArray> = mutableListOf() | ||
| } | ||
|
|
||
| private val buffers = ConcurrentHashMap<String, DeviceBuffer>() | ||
|
|
||
| /** | ||
| * Buffer a single prepared-write chunk for [deviceKey], placing [value] at | ||
| * [offset] within that device's growing payload. | ||
| * | ||
| * Chunks are written at their byte offset, so reassembly is correct even if | ||
| * chunks are delivered out of order. Returns true if the chunk was accepted, | ||
| * or false if it was rejected: a negative offset, or a write that would push | ||
| * the payload past [maxPayloadSize]. On overflow the device's buffer is | ||
| * marked so that [execute] yields null and the oversized payload is dropped. | ||
| */ | ||
| fun append(deviceKey: String, offset: Int, value: ByteArray): Boolean { | ||
| if (offset < 0) return false | ||
| val buf = buffers.getOrPut(deviceKey) { DeviceBuffer() } | ||
| synchronized(buf) { | ||
| if (buf.overflowed) return false | ||
| val end = offset.toLong() + value.size.toLong() | ||
| if (end > maxPayloadSize.toLong()) { | ||
| // Drop what we have and remember the overflow until the write is | ||
| // finalized/cancelled, so we never allocate beyond the cap. | ||
| buf.overflowed = true | ||
| buf.data = ByteArray(0) | ||
| buf.length = 0 | ||
| buf.covered.clear() | ||
| return false | ||
| } | ||
| val endInt = end.toInt() | ||
| if (endInt > buf.data.size) { | ||
| val newCapacity = maxOf(endInt, buf.data.size * 2).coerceAtMost(maxPayloadSize) | ||
| buf.data = buf.data.copyOf(maxOf(newCapacity, endInt)) | ||
| } | ||
| System.arraycopy(value, 0, buf.data, offset, value.size) | ||
| if (endInt > buf.length) buf.length = endInt | ||
| if (value.isNotEmpty()) addCovered(buf.covered, offset, endInt) | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Merge the range [start, end) into [ranges], keeping it sorted and coalesced | ||
| * so [execute] can check whether the payload was fully covered. | ||
| */ | ||
| private fun addCovered(ranges: MutableList<IntArray>, start: Int, end: Int) { | ||
| if (end <= start) return | ||
| ranges.add(intArrayOf(start, end)) | ||
| ranges.sortBy { it[0] } | ||
| val merged = ArrayList<IntArray>(ranges.size) | ||
| for (r in ranges) { | ||
| val last = merged.lastOrNull() | ||
| if (last != null && r[0] <= last[1]) { | ||
| last[1] = maxOf(last[1], r[1]) | ||
| } else { | ||
| merged.add(r) | ||
| } | ||
| } | ||
| ranges.clear() | ||
| ranges.addAll(merged) | ||
| } | ||
|
|
||
| /** | ||
| * Finalize the prepared write for [deviceKey] and return the reassembled | ||
| * payload, or null if nothing was buffered or the buffer overflowed. The | ||
| * device's buffer is always removed. | ||
| */ | ||
| fun execute(deviceKey: String): ByteArray? { | ||
| val buf = buffers.remove(deviceKey) ?: return null | ||
| synchronized(buf) { | ||
| if (buf.overflowed || buf.length == 0) return null | ||
| // Reject a payload with a hole: a missing chunk would otherwise be | ||
| // handed up as zero-filled and parse as if it were complete. Require a | ||
| // single contiguous run covering [0, length). | ||
| if (buf.covered.size != 1 || buf.covered[0][0] != 0 || buf.covered[0][1] != buf.length) { | ||
| return null | ||
| } | ||
| return buf.data.copyOf(buf.length) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Discard any buffered chunks for [deviceKey]. Used when a prepared write is | ||
| * cancelled or when the device disconnects, to avoid leaking buffers. | ||
| */ | ||
| fun cancel(deviceKey: String) { | ||
| buffers.remove(deviceKey) | ||
| } | ||
|
|
||
| /** | ||
| * Drop every device's buffered chunks. Used when the GATT server shuts down, since a | ||
| * disconnect that happens after the server is already marked inactive never reaches the | ||
| * per-device cancel path. | ||
| */ | ||
| fun clearAll() { | ||
| buffers.clear() | ||
| } | ||
|
|
||
| /** Number of devices currently holding buffered chunks (diagnostics/tests). */ | ||
| fun activeDeviceCount(): Int = buffers.size | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
stop()setsisActive = falsebefore 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 instop(), or perform this per-device cleanup before the callback'sisActiveguard.AGENTS.md reference: AGENTS.md:L63-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already handled:
stop()callspreparedWriteBuffer.clearAll()before theisActiveguard (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 atSTATE_DISCONNECTED(not behind the guard), so a prepared write can't survive a stop/restart.