diff --git a/spec/modules/NOTES.md b/spec/modules/NOTES.md new file mode 100644 index 000000000..0fad99561 --- /dev/null +++ b/spec/modules/NOTES.md @@ -0,0 +1,155 @@ +# Design Notes + +Non-bug observations from module specs that are worth tracking. These remain documented in their respective module specs — this file serves as an index. + +## Backend Observations + +### N-01: SNotifier path doesn't cache + +**Location**: `Simplex.Messaging.Server.QueueStore.Postgres` — `getQueues_` SNotifier branch +**Description**: The SRecipient path caches loaded queues via `cacheRcvQueue` with double-check locking. The SNotifier path does NOT cache — it uses a stale TMap snapshot and `maybe (mkQ False rId qRec) pure`, so concurrent loads for the same notifier can create duplicate ephemeral queue objects. Functionally correct but wasteful. +**Module spec**: [QueueStore/Postgres.md](Simplex/Messaging/Server/QueueStore/Postgres.md) + +### N-02: assertUpdated error conflation + +**Location**: `Simplex.Messaging.Server.QueueStore.Postgres` — `assertUpdated` +**Description**: `assertUpdated` returns `AUTH` for zero-rows-affected. This is the same error code used for "not found" (via `readQueueRecIO`) and "duplicate" (via `handleDuplicate`). The actual cause — stale cache, deleted queue, or constraint violation — is indistinguishable in logs. +**Module spec**: [QueueStore/Postgres.md](Simplex/Messaging/Server/QueueStore/Postgres.md) + +## Design Characteristics + +### N-03: RCVerifiedInvitation constructor exported + +**Location**: `Simplex.RemoteControl.Invitation` — `RCVerifiedInvitation` +**Description**: `RCVerifiedInvitation` is a newtype with constructor exported via `(..)`. It can be constructed without calling `verifySignedInvitation`, bypassing signature verification. The trust boundary is conventional, not enforced by the type system. `connectRCCtrl` accepts only `RCVerifiedInvitation`. +**Module spec**: [RemoteControl/Invitation.md](Simplex/RemoteControl/Invitation.md) + +### N-04: smpEncode Word16 silent truncation + +**Location**: `Simplex.Messaging.Encoding` — `Encoding Word16` instance +**Description**: `smpEncode` for ByteString uses a 1-byte length prefix. Maximum encodable length is 255 bytes. Longer values silently wrap via `w2c . fromIntegral`. Callers must ensure ByteStrings fit or use `Large`. +**Module spec**: [Encoding.md](Simplex/Messaging/Encoding.md) + +### N-05: writeIORef for period stats — not atomic + +**Location**: `Simplex.Messaging.Server.Stats` — `setPeriodStats` +**Description**: Uses `writeIORef` (not atomic). Only safe during router startup when no other threads are running. If called concurrently, period data could be corrupted. +**Module spec**: [Server/Stats.md](Simplex/Messaging/Server/Stats.md) + +### N-06: setStatsByServer orphans old TVars + +**Location**: `Simplex.Messaging.Notifications.Server.Stats` — `setStatsByServer` +**Description**: Builds a fresh `Map Text (TVar Int)` in IO, then atomically replaces the TMap's root TVar. Old per-router TVars are not reused — any other thread holding a reference from a prior `TM.lookupIO` would modify an orphaned counter. Called at startup, but lacks the explicit "not thread safe" comment. +**Module spec**: [Notifications/Server/Stats.md](Simplex/Messaging/Notifications/Server/Stats.md) + +### N-07: Lazy.unPad doesn't validate data length + +**Location**: `Simplex.Messaging.Crypto.Lazy` — `unPad` / `splitLen` +**Description**: `splitLen` does not validate that the remaining data is at least `len` bytes — `LB.take len` silently returns a shorter result. The source comment notes this is intentional to avoid consuming all lazy chunks for validation. +**Module spec**: [Crypto/Lazy.md](Simplex/Messaging/Crypto/Lazy.md) + +### N-08: Batched commands have no timeout-based expiry + +**Location**: `Simplex.Messaging.Client` — `sendBatch` +**Description**: Batched commands are written with `Nothing` as the request parameter — the send thread skips the `pending` flag check. Individual commands have timeout-based expiry. If the router stops returning results, batched commands can block the send queue indefinitely. +**Module spec**: [Client.md](Simplex/Messaging/Client.md) + +### N-09: Postgres MsgStore nanosecond precision + +**Location**: `Simplex.Messaging.Server.MsgStore.Postgres` — `toMessage` +**Description**: `MkSystemTime ts 0` constructs timestamps with zero nanoseconds. Only whole seconds are stored. Messages read from Postgres have coarser timestamps than STM/Journal stores. Not a practical issue — timestamps are typically rounded to hours or days. +**Module spec**: [Server/MsgStore/Postgres.md](Simplex/Messaging/Server/MsgStore/Postgres.md) + +### N-10: MsgStore Postgres — error stubs crash at runtime + +**Location**: `Simplex.Messaging.Server.MsgStore.Postgres` — multiple `MsgStoreClass` methods +**Description**: Multiple `MsgStoreClass` methods are `error "X not used"`. Required by the type class but not applicable to Postgres. Calling any at runtime crashes. Safe because Postgres overrides the relevant default methods, but a new caller using the wrong method would crash with no compile-time warning. +**Module spec**: [Server/MsgStore/Postgres.md](Simplex/Messaging/Server/MsgStore/Postgres.md) + +### N-11: strP default assumes base64url for all types + +**Location**: `Simplex.Messaging.Encoding.String` — `StrEncoding` class default +**Description**: The `MINIMAL` pragma allows defining only `strDecode` without `strP`. The default `strP = strDecode <$?> base64urlP` assumes input is base64url-encoded for any type. A new `StrEncoding` instance that defines only `strDecode` for non-base64 data would get a broken parser. +**Module spec**: [Encoding/String.md](Simplex/Messaging/Encoding/String.md) + +## Silent Behaviors + +Intentional design choices that are correct but non-obvious. A code modifier who doesn't know these could introduce bugs. + +### N-12: Service signing silently skipped on empty authenticator + +**Location**: `Simplex.Messaging.Client` — service signature path +**Description**: The service signature is only added when the entity authenticator is non-empty. If authenticator generation fails silently (returns empty bytes), service signing is silently skipped. +**Module spec**: [Client.md](Simplex/Messaging/Client.md) + +### N-13: stmDeleteNtfToken — nonexistent token indistinguishable from empty + +**Location**: `Simplex.Messaging.Notifications.Server.Store` — `stmDeleteNtfToken` +**Description**: If the token ID doesn't exist in the `tokens` map, the registration-cleanup branch is skipped and the function returns an empty list. The caller cannot distinguish "deleted a token with no subscriptions" from "token never existed." +**Module spec**: [Notifications/Server/Store.md](Simplex/Messaging/Notifications/Server/Store.md) + +### N-14: createCommand silently drops commands for deleted connections + +**Location**: `Simplex.Messaging.Agent.Store.AgentStore` — `createCommand` +**Description**: When `createCommand` encounters a constraint violation (the referenced connection was already deleted), it logs the error and returns successfully. Commands targeting deleted connections are silently dropped. +**Module spec**: [Agent/Store/AgentStore.md](Simplex/Messaging/Agent/Store/AgentStore.md) + +### N-15: Redirect chain loading errors silently swallowed + +**Location**: `Simplex.Messaging.Agent.Store.AgentStore` +**Description**: When loading redirect chains, errors loading individual redirect files are silently swallowed via `either (const $ pure Nothing) (pure . Just)`. Prevents a corrupt redirect from blocking access to the main file. +**Module spec**: [Agent/Store/AgentStore.md](Simplex/Messaging/Agent/Store/AgentStore.md) + +### N-16: BLOCKED encoded as AUTH for old XFTP clients + +**Location**: `Simplex.FileTransfer.Protocol` — `encodeProtocol` +**Description**: If the protocol version is below `blockedFilesXFTPVersion`, a `BLOCKED` result is encoded as `AUTH` instead. The blocking information (reason) is permanently lost for these clients. +**Module spec**: [FileTransfer/Protocol.md](Simplex/FileTransfer/Protocol.md) + +### N-17: restore_messages three-valued logic with implicit default + +**Location**: `Simplex.Messaging.Server.Main` — INI config +**Description**: The `restore_messages` INI setting has three-valued logic: explicit "on" → restore, explicit "off" → skip, missing → inherits from `enable_store_log`. This implicit default is not captured in the type system — callers see `Maybe Bool`. +**Module spec**: [Server/Main.md](Simplex/Messaging/Server/Main.md) + +### N-18: Stats format migration permanently loses precision + +**Location**: `Simplex.Messaging.Server.Stats` — `strP` for `ServerStatsData` +**Description**: The parser handles multiple format generations. Old format `qDeleted=` is read as `(value, 0, 0)`. `qSubNoMsg` is parsed and discarded. `subscribedQueues` is parsed but replaced with empty data. Data loaded from old formats is coerced — precision is permanently lost. +**Module spec**: [Server/Stats.md](Simplex/Messaging/Server/Stats.md) + +### N-19: resubscribe exceptions silently lost + +**Location**: `Simplex.Messaging.Notifications.Server` — `resubscribe` +**Description**: `resubscribe` is launched via `forkIO` before `raceAny_` starts — not part of the `raceAny_` group. Most exceptions are silently lost per `forkIO` semantics. `ExitCode` exceptions are special-cased by GHC's runtime and do propagate. +**Module spec**: [Notifications/Server.md](Simplex/Messaging/Notifications/Server.md) + +### N-20: closeSMPClientAgent worker cancellation is fire-and-forget + +**Location**: `Simplex.Messaging.Client.Agent` — `closeSMPClientAgent` +**Description**: Executes in order: set `active = False`, close all client connections, swap workers map to empty and fork cancellation threads. Cancel threads use `uninterruptibleCancel` but are fire-and-forget — the function may return before all workers are cancelled. +**Module spec**: [Client/Agent.md](Simplex/Messaging/Client/Agent.md) + +### N-21: APNS unknown 410 reasons trigger retry instead of permanent failure + +**Location**: `Simplex.Messaging.Notifications.Server.Push.APNS` +**Description**: Unknown 410 (Gone) reasons fall through to `PPRetryLater`, while unknown 400 and 403 reasons fall through to `PPResponseError`. An unexpected APNS 410 reason string triggers retry rather than permanent failure. +**Module spec**: [Notifications/Server/Push/APNS.md](Simplex/Messaging/Notifications/Server/Push/APNS.md) + +### N-22: NTInvalid/NTExpired tokens can create subscriptions + +**Location**: `Simplex.Messaging.Notifications.Protocol` — token status permissions +**Description**: Token status `NTInvalid` allows subscription commands (SNEW, SCHK, SDEL). A TODO comment explains: invalidation can happen after verification, and existing subscriptions should remain manageable. `NTExpired` is also permitted. +**Module spec**: [Notifications/Protocol.md](Simplex/Messaging/Notifications/Protocol.md) + +### N-23: removeInactiveTokenRegistrations doesn't clean up empty inner maps + +**Location**: `Simplex.Messaging.Notifications.Server.Store` — `stmRemoveInactiveTokenRegistrations` +**Description**: `stmDeleteNtfToken` checks whether inner TMap is empty after removal and cleans up the outer key. `stmRemoveInactiveTokenRegistrations` does not — surviving active tokens' registrations remain, but empty inner maps can persist. +**Module spec**: [Notifications/Server/Store.md](Simplex/Messaging/Notifications/Server/Store.md) + +### N-24: cbNonce silently truncates or pads + +**Location**: `Simplex.Messaging.Crypto` — `cbNonce` +**Description**: If the input is longer than 24 bytes, it is silently truncated. If shorter, it is silently padded. No error is raised. Callers must ensure correct length. +**Module spec**: [Crypto.md](Simplex/Messaging/Crypto.md) diff --git a/spec/modules/Simplex/Messaging/Agent/Store/AgentStore.md b/spec/modules/Simplex/Messaging/Agent/Store/AgentStore.md index d1271a6d3..59d7c2010 100644 --- a/spec/modules/Simplex/Messaging/Agent/Store/AgentStore.md +++ b/spec/modules/Simplex/Messaging/Agent/Store/AgentStore.md @@ -75,10 +75,6 @@ Generates random 12-byte IDs (base64url encoded) and retries up to 3 times on co First clears primary flag on all queues in the connection, then sets it on the target queue. Also clears `replace_*_queue_id` on the new primary — this completes the queue rotation by removing the "replacing" marker. -## checkConfirmedSndQueueExists_ — dpPostgres typo - -The CPP guard reads `#if defined(dpPostgres)` (note `dp` instead of `db`). This means the `FOR UPDATE` clause is never included for any backend. The check still works correctly for SQLite (single-writer model) but on PostgreSQL the query runs without row locking, which could allow a TOCTOU race between checking and inserting. - ## createCommand — silent drop for deleted connections When `createCommand` encounters a constraint violation (the referenced connection was already deleted), it logs the error and returns successfully rather than throwing. This means commands targeting deleted connections are silently dropped. The rationale: the connection is already gone, so there's nothing useful to do with the error. diff --git a/spec/modules/Simplex/Messaging/Client.md b/spec/modules/Simplex/Messaging/Client.md index f23d5a005..4e97c9a5c 100644 --- a/spec/modules/Simplex/Messaging/Client.md +++ b/spec/modules/Simplex/Messaging/Client.md @@ -42,7 +42,7 @@ When `corrId` is empty, the message is an `STEvent` (router-initiated). When non ## nonBlockingWriteTBQueue — fork on full -If `tryWriteTBQueue` returns `False`, a new thread is forked for the blocking write. No backpressure mechanism — under sustained overload, thread count grows without bound. This is a deliberate tradeoff: the caller never blocks (preventing deadlock between send and process threads), at the cost of potential unbounded thread creation. +If `tryWriteTBQueue` returns `False` (queue full), a new thread is forked for the blocking write. The caller never blocks, preventing deadlock between send and process threads. ## Batch commands do not expire diff --git a/spec/modules/Simplex/Messaging/Encoding.md b/spec/modules/Simplex/Messaging/Encoding.md index 8db63d0cc..984498dd4 100644 --- a/spec/modules/Simplex/Messaging/Encoding.md +++ b/spec/modules/Simplex/Messaging/Encoding.md @@ -14,8 +14,6 @@ The two encoding classes share some instances (`Char`, `Bool`, `SystemTime`) but **Length prefix is 1 byte.** Maximum encodable length is 255 bytes. If a ByteString exceeds 255 bytes, the length silently wraps via `w2c . fromIntegral` — a 300-byte string encodes length as 44 (300 mod 256). Callers must ensure ByteStrings fit in 255 bytes, or use `Large` for longer values. -**Security**: silent truncation means a caller encoding untrusted input without length validation could produce a malformed message where the decoder reads fewer bytes than were intended, then misparses the remainder as the next field. - ## Large 2-byte length prefix (`Word16`). Use for ByteStrings that may exceed 255 bytes. Maximum 65535 bytes. @@ -36,10 +34,6 @@ Sequential concatenation with no separators. Works because each element's encodi Only seconds are encoded (as Int64); nanoseconds are discarded on encode and set to 0 on decode. -## String instance - -`smpEncode` goes through `B.pack`, which silently truncates any Unicode character above codepoint 255 to its lowest byte. A String containing non-Latin-1 characters is silently corrupted on encode with no error. Same issue exists in the `StrEncoding String` instance — see [Simplex.Messaging.Encoding.String](./Encoding/String.md#string-instance). - ## smpEncodeList / smpListP 1-byte length prefix for lists — same 255-item limit as ByteString's 255-byte limit. diff --git a/spec/modules/Simplex/Messaging/Encoding/String.md b/spec/modules/Simplex/Messaging/Encoding/String.md index 1e60295b8..378bed11f 100644 --- a/spec/modules/Simplex/Messaging/Encoding/String.md +++ b/spec/modules/Simplex/Messaging/Encoding/String.md @@ -21,7 +21,7 @@ Encodes as base64url. The parser (`strP`) only accepts non-empty strings — emp ## String instance -Inherits from ByteString via `B.pack` / `B.unpack`. Only Char8 (Latin-1) characters round-trip; `B.pack` truncates unicode codepoints above 255. The source comment warns about this. +Inherits from ByteString via `B.pack` / `B.unpack`. Only Char8 (Latin-1) characters round-trip. ## strToJSON / strParseJSON diff --git a/spec/modules/Simplex/Messaging/Notifications/Server/Store.md b/spec/modules/Simplex/Messaging/Notifications/Server/Store.md index d9deedbf4..4259b44c7 100644 --- a/spec/modules/Simplex/Messaging/Notifications/Server/Store.md +++ b/spec/modules/Simplex/Messaging/Notifications/Server/Store.md @@ -20,7 +20,7 @@ When a token is activated, `stmRemoveInactiveTokenRegistrations` removes ALL oth ### 4. tokenLastNtfs accumulates via prepend -New notifications are prepended to the `NonEmpty PNMessageData` list via `(<|)`. The list is unbounded in the STM store — bounding is handled at the push delivery layer (the Postgres store limits to 6). +New notifications are prepended to the `NonEmpty PNMessageData` list via `(<|)`. ### 5. stmDeleteNtfToken prunes empty registration maps @@ -46,9 +46,9 @@ When `stmDeleteNtfToken` removes a token, it deletes the entry from the inner `T When `stmDeleteNtfSubscription` removes a subscription, it deletes the `subId` from the token's `Set NtfSubscriptionId` in `tokenSubscriptions` but never checks whether the set became empty. Tokens with all subscriptions individually deleted accumulate empty set entries — these are only cleaned up when the token itself is deleted via `deleteTokenSubs`. -### 11. stmSetNtfService — asymmetric cleanup with Postgres store +### 11. stmSetNtfService — key-value service association -`stmSetNtfService` uses `maybe TM.delete TM.insert` to either remove or set the service association for an SMP router. This is purely a key-value update with no cascading effects on subscriptions. The Postgres store's `removeServiceAndAssociations` handles subscription cleanup separately, meaning the STM and Postgres stores have **different cleanup semantics** for service removal. +`stmSetNtfService` uses `maybe TM.delete TM.insert` to either remove or set the service association for an SMP router. This is purely a key-value update with no cascading effects on subscriptions. ### 12. Subscription index triple-write invariant diff --git a/spec/modules/Simplex/Messaging/Notifications/Server/Store/Postgres.md b/spec/modules/Simplex/Messaging/Notifications/Server/Store/Postgres.md index bde863eb6..1950ee1e1 100644 --- a/spec/modules/Simplex/Messaging/Notifications/Server/Store/Postgres.md +++ b/spec/modules/Simplex/Messaging/Notifications/Server/Store/Postgres.md @@ -83,7 +83,7 @@ The `insertServer` fallback uses `ON CONFLICT ... DO UPDATE SET smp_host = EXCLU ### 18. deleteNtfToken string_agg with hex parsing -`deleteNtfToken` uses `string_agg(s.smp_notifier_id :: TEXT, ',')` to aggregate `BYTEA` notifier IDs into comma-separated text, then parses with `parseByteaString` which drops the `\x` prefix and hex-decodes. `mapMaybe` silently drops any IDs that fail hex decoding, which could mask data corruption. +`deleteNtfToken` uses `string_agg(s.smp_notifier_id :: TEXT, ',')` to aggregate `BYTEA` notifier IDs into comma-separated text, then parses with `parseByteaString` which drops the `\x` prefix and hex-decodes. ### 19. withPeriodicNtfTokens streams with DB.fold diff --git a/spec/modules/Simplex/Messaging/Server/MsgStore/Postgres.md b/spec/modules/Simplex/Messaging/Server/MsgStore/Postgres.md index eaeca3b90..a69e2e9ee 100644 --- a/spec/modules/Simplex/Messaging/Server/MsgStore/Postgres.md +++ b/spec/modules/Simplex/Messaging/Server/MsgStore/Postgres.md @@ -52,6 +52,3 @@ Creates a temp table with aggregated message stats, then updates `msg_queues` in `deleteQueueSize` calls `getQueueSize` BEFORE `deleteStoreQueue`. The returned size is the count at query time — a concurrent `writeMsg` between the size query and the delete means the reported size is stale. This is acceptable because the size is used for statistics, not for correctness. -## unsafeMaxLenBS - -`toMessage` uses `C.unsafeMaxLenBS` to bypass the `MaxLen` length check on message bodies read from the database. A TODO comment questions this choice. If the database contains oversized data, the length invariant is silently violated. diff --git a/spec/modules/Simplex/Messaging/Server/QueueStore/Postgres.md b/spec/modules/Simplex/Messaging/Server/QueueStore/Postgres.md index f97acaa2d..39d833169 100644 --- a/spec/modules/Simplex/Messaging/Server/QueueStore/Postgres.md +++ b/spec/modules/Simplex/Messaging/Server/QueueStore/Postgres.md @@ -6,7 +6,7 @@ ## addQueue_ — no in-memory duplicate check, relies on DB constraint -See comment on `addQueue_`: "Not doing duplicate checks in maps as the probability of duplicates is very low." The STM implementation checks all four ID maps before insertion and returns `DUPLICATE_`. The Postgres implementation skips this and relies on `UniqueViolation` from the DB, which `handleDuplicate` maps to `AUTH`, not `DUPLICATE_`. The same logical error produces different error codes depending on the store backend. +See comment on `addQueue_`: "Not doing duplicate checks in maps as the probability of duplicates is very low." The Postgres implementation relies on `UniqueViolation` from the DB rather than pre-checking in-memory maps. ## addQueue_ — non-atomic cache updates @@ -56,17 +56,13 @@ Re-securing with the same key falls through the verify function to `pure ()`, th (1) **Cache check**: `checkCachedNotifier` acquires a per-notifier-ID lock via `notifierLocks`, then checks `TM.memberIO`. Returns `DUPLICATE_`. (2) **Queue lock**: Via `withQueueRec`, prevents concurrent modifications to the same queue. (3) **Database constraint**: `handleDuplicate` catches `UniqueViolation`, returns `AUTH`. Same duplicate, different error codes depending on whether cache was warm. The `notifierLocks` map grows unboundedly — locks are never removed except when the queue is deleted. -## addQueueNotifier — always clears notification service - -The SQL UPDATE always sets `ntf_service_id = NULL` when adding/replacing a notifier. The previous notifier's service association is silently lost. The STM implementation additionally calls `removeServiceQueue` to update service-level tracking; the Postgres version does not. - ## rowToQueueRec — link data replaced with empty stubs The standard `queueRecQuery` does NOT select `fixed_data` and `user_data` columns. When converting to `QueueRec`, link data is stubbed: `(,(EncDataBytes "", EncDataBytes "")) <$> linkId_`. Actual link data is loaded on demand via `getQueueLinkData`. Any code reading `queueData` from a cached `QueueRec` without going through `getQueueLinkData` sees empty bytes. The separate `rowToQueueRecWithData` (used by `foldQueueRecs` with `withData = True`) includes real data. ## getCreateService — serialization via serviceLocks -Entire operation wrapped in `withLockMap (serviceLocks st) fp`, serializing all creation/lookup for the same certificate fingerprint. Inside the lock: SELECT by `service_cert_hash`, if not found attempt INSERT catching `UniqueViolation`. The `serviceLocks` map grows unboundedly — no cleanup mechanism. +Entire operation wrapped in `withLockMap (serviceLocks st) fp`, serializing all creation/lookup for the same certificate fingerprint. Inside the lock: SELECT by `service_cert_hash`, if not found attempt INSERT catching `UniqueViolation`. ## batchInsertQueues — COPY protocol with manual CSV serialization diff --git a/spec/modules/Simplex/Messaging/Util.md b/spec/modules/Simplex/Messaging/Util.md index 3b9fd3777..d89e27bf1 100644 --- a/spec/modules/Simplex/Messaging/Util.md +++ b/spec/modules/Simplex/Messaging/Util.md @@ -47,6 +47,3 @@ Runs all actions concurrently, waits for any one to complete, then cancels all o Handles `Int64` delays exceeding `maxBound :: Int` (~2147 seconds on 32-bit) by looping in chunks. Necessary because `threadDelay` takes `Int`, not `Int64`. -## toChunks - -Precondition: `n > 0` (comment-only, not enforced). Passing `n = 0` causes infinite loop. diff --git a/spec/modules/Simplex/RemoteControl/Client.md b/spec/modules/Simplex/RemoteControl/Client.md index 55fd05bc1..84a5d2dca 100644 --- a/spec/modules/Simplex/RemoteControl/Client.md +++ b/spec/modules/Simplex/RemoteControl/Client.md @@ -30,7 +30,7 @@ The session key combines DH and post-quantum KEM via `kemHybridSecret`: `SHA3_25 2. Application displays session code for user verification → calls `confirmCtrlSession` with `True`/`False` 3. If confirmed, `runSession` proceeds with hello exchange → second `RCStepTMVar` resolved with session -`confirmCtrlSession` does a double `putTMVar` — the first signals the decision, the second blocks until the session thread does `takeTMVar` (synchronization point). See TODO in source: no timeout on this wait. +`confirmCtrlSession` does a double `putTMVar` — the first signals the decision, the second blocks until the session thread does `takeTMVar` (synchronization point). ## TLS hooks — single-session enforcement diff --git a/spec/modules/Simplex/RemoteControl/Discovery.md b/spec/modules/Simplex/RemoteControl/Discovery.md index 52c861c79..22fd9d6d6 100644 --- a/spec/modules/Simplex/RemoteControl/Discovery.md +++ b/spec/modules/Simplex/RemoteControl/Discovery.md @@ -12,8 +12,6 @@ Enumerates network interfaces and filters out non-routable addresses (0.0.0.0, b `joinMulticast` / `partMulticast` use a shared `TMVar Int` counter to track active listeners. Multicast group membership is per-host (not per-process — see comment in Multicast.hsc), so the counter ensures `IP_ADD_MEMBERSHIP` is called only when transitioning from 0→1 listeners and `IP_DROP_MEMBERSHIP` only when transitioning from 1→0. If `setMembership` fails, the counter is restored to its previous value and the error is logged (not thrown). -**TMVar hazard**: Both functions take the counter from the TMVar unconditionally but only put it back in the 0-or-1 branches. If `joinMulticast` is called when the counter is already >0, or `partMulticast` when >1, the TMVar is left empty and subsequent accesses will deadlock. In practice this is safe because `withListener` serializes access through a single `TMVar Int`, but the abstraction does not protect against concurrent use. - ## startTLSServer — ephemeral port support When `port_` is `Nothing`, passes `"0"` to `startTCPServer`, which causes the OS to assign an ephemeral port. The assigned port is read via `socketPort` and communicated back through the `startedOnPort` TMVar. On any startup error, `setPort Nothing` is signalled so callers don't block indefinitely on the TMVar. diff --git a/spec/modules/Simplex/RemoteControl/Invitation.md b/spec/modules/Simplex/RemoteControl/Invitation.md index 3f65ec46c..a12a12f99 100644 --- a/spec/modules/Simplex/RemoteControl/Invitation.md +++ b/spec/modules/Simplex/RemoteControl/Invitation.md @@ -15,7 +15,7 @@ Verification in `verifySignedInvitation` mirrors this: `ssig` is verified agains ## Invitation URI format -The `xrcp:/` scheme uses the SMP-style pattern: CA fingerprint as userinfo (`ca@host:port`), query parameters after `#/?`. The `app` field is raw JSON encoded in a query parameter. `RCInvitation`'s parser uses `parseSimpleQuery` + `lookup` (order-independent), but `RCSignedInvitation`'s parser uses `B.breakSubstring "&ssig="` which assumes the signatures appear at a fixed position — see TODO in source on `RCSignedInvitation`'s `strP`. +The `xrcp:/` scheme uses the SMP-style pattern: CA fingerprint as userinfo (`ca@host:port`), query parameters after `#/?`. The `app` field is raw JSON encoded in a query parameter. `RCInvitation`'s parser uses `parseSimpleQuery` + `lookup` (order-independent). ## RCVerifiedInvitation — newtype trust boundary diff --git a/spec/modules/Simplex/RemoteControl/Types.md b/spec/modules/Simplex/RemoteControl/Types.md index ad165f442..f752d465f 100644 --- a/spec/modules/Simplex/RemoteControl/Types.md +++ b/spec/modules/Simplex/RemoteControl/Types.md @@ -28,4 +28,4 @@ This module defines the data types for the XRCP (remote control) protocol, which ## IpProbe — unused discovery type -`IpProbe` is defined with `Encoding` instance but not used anywhere in the current codebase. It appears to be a placeholder for a planned IP discovery mechanism. Note: the `smpP` parser has a precedence bug — `IpProbe <$> (smpP <* "I") *> smpP` parses as `(IpProbe <$> (smpP <* "I")) *> smpP`, which discards the `IpProbe` wrapper. This has never manifested because the type is unused. +`IpProbe` is defined with `Encoding` instance but not used anywhere in the current codebase. It appears to be a placeholder for a planned IP discovery mechanism.