Repository navigation
Harden capture file open and import - #156
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| var device = | ||
| new CaptureDeviceContextModel( | ||
| (DeviceType)record.Int("DeviceType"), | ||
| record.Str("DeviceName"), | ||
| (CaptureType)record.Int("CaptureType"), | ||
| true, | ||
| null, | ||
| hostListPolicy: HostListPolicy); |
There was a problem hiding this comment.
[P2] Detach staged network controllers when reusing a read-only tab
Constructing each staged CaptureDeviceContextModel calls NetworkTraceCaptureController.Initialize(), which subscribes to the active table's RowAdded. When an existing tab is reused, ReplaceLoadedCapturesFrom assigns the staged controller's packet collection to the existing controller, but the staged controller remains subscribed (Dispose() is also empty). Both handlers therefore append to the same collection.
Repro: open a network .xmatcap, reopen it (or open another capture of the same device), then capture one live packet for that device. The reloaded read-only tab contains two entries for one inserted row, visible when the view refreshes. The saved database is not duplicated. Read-only tabs receiving matching live packets was pre-existing; the extra subscriber and double entries are introduced here. Repeated reloads also retain additional subscribers and their packet collections, adding a SQLite read per subscriber for every live packet.
Please detach discarded staged controllers after transfer and on failed loads, or stage .xmatcap data without live subscriptions. Avoid disabling subscriptions for every read-only network controller, since ETL import relies on RowAdded.
Validation: Debug build and all 1,471 existing tests passed. An additional focused regression test passes without a reload and fails after a reload with two packet entries instead of one.
There was a problem hiding this comment.
Thanks, Steve, good catch and a very clear repro. Fixed in 3357889: LoadDataCaptures now disposes every staged controller that doesn't become a tab, both after the transfer into a reused tab and when a load fails partway. NetworkTraceCaptureController.Dispose() unsubscribes from RowAdded and keeps the packet collection, which the reused tab now shows. Open tabs, including .etl imports, keep receiving rows. Closing a network tab also detaches it now, but only once the tab is actually removed. NetworkCaptureReloadTests covers your repro, repeated reloads, a failed load, an .etl import tab and both ways a close can fail.
There was a problem hiding this comment.
[P2 follow-up] An already-dispatched RowAdded callback can still duplicate a packet during reload
Rechecked 3357889: the original steady-state leak and reproduction are fixed. However, Dispose() only unsubscribes; it cannot remove the staged handler from a multicast delegate snapshot already being invoked. ReplaceLoadedCapturesFrom shares the staged packet collection with the existing controller (lines 375-376), so both captured handlers can still append the same row after the staged controller is disposed.
Deterministic reproduction with the real live table:
- During the existing controller's
NetworkTracePacketschange notification in a reload, start oneliveTable.AddRowon a worker. - Pause that
RowAddedinvocation in a subscriber registered before the controllers. Its delegate snapshot now contains both the existing and staged controller handlers. - Let
LoadDataCapturesfinish and dispose the staged controller, then resume delivery. The reused tab contains two packets for one inserted row. The identical paused-delivery control without reloading contains one.
Opening captures is allowed while live capture runs (CanModifyCaptureData does not require captures to be stopped), so this overlap is reachable through normal UI use. This is a residual in-flight delivery race, not a remaining permanent subscription leak; the duplicate is in the tab's packet collection, not an additional database insert.
Please prevent already-dispatched staged callbacks from mutating the transferred collection, or keep .xmatcap staging controllers unsubscribed until they actually become tabs, while preserving ETL import subscriptions.
Validation: all 52 existing network-lifecycle/capture-model tests and the original reproduction pass in Debug and Release. The additional in-flight reload case fails in both configurations with two entries instead of one; its no-reload control passes.
There was a problem hiding this comment.
Thanks, Steve, agreed. Fixed in 1ab4c74 with your second option: .xmatcap staging controllers no longer subscribe to RowAdded. A new tab attaches only after the load commits, and transfer sources, failed loads and rolled-back tabs never attach, so a staged handler is never in a delivery's handler list. .etl imports and live captures still subscribe when they're created. The trade-off, now in docs/capture-files.md: a loaded tab shows a running capture's packets only from when it opens or reloads; the live tab and the saved data keep every packet. NetworkCaptureReloadTests adds your in-flight repro (two entries before, one after) with its no-reload control, plus attach timing, a rejected add, reused row IDs and the live tab during a reload.
46d3bfb to
23c7379
Compare
XMAT checks a .SAZ archive's directory before reading it and refuses an archive that lists more than 750,064 files, has a directory larger than 80 MiB, spans several disks or has an inconsistent directory. Each request or response may have at most 16,384 header lines and 16 MiB of header data; the limits apply while an entry decompresses, and a larger message stops the import rather than being truncated. Exports can retain longer headers, and XMAT refuses to import such an archive. Malformed header lines and incomplete frames are skipped, and the status bar notes it. A request without a usable Host header is imported without a host, and analysis keeps its path instead of failing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 270d0c0c-8578-4bd6-aa1d-c32a56075710
XMAT checks a .XMATCAP file's database schema before SQLite reads it and opens the file read-only. Only the known tables and columns are accepted, plus extra untyped, TEXT or INTEGER columns that may hold NULL. Any other table, index, view, trigger, constraint, default or generated column, a schema beyond the safety limits, SQLite journal or write-ahead log files next to the capture (-journal, -wal or -shm), another program having the file open for writing, or a location where Windows cannot report a normalized path (such as some network shares) make XMAT refuse the file without changing it, those files or the open tabs. While a capture is open, other programs can read it but not change it, and SQLite reads only the checked file. A loaded capture reuses the read-only tab of the same device and shares the session's Block and Bypass lists, and closing the last tab of a loaded file releases it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 270d0c0c-8578-4bd6-aa1d-c32a56075710
HasScheme returns early for null or empty input, the protocol filter name concatenates the enum without an explicit ToString, and capture loading filters the tables a device owns with Where. Behavior is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 270d0c0c-8578-4bd6-aa1d-c32a56075710
The writer-access test in SqLtSchemaPreflightTests releases its view, mapping and writer through scoped using statements, in the same order and before it checks that the capture folder is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 270d0c0c-8578-4bd6-aa1d-c32a56075710
Reopening a network capture into an existing tab, or a load that fails partway, no longer leaves the discarded network controllers subscribed to the live packet table. A reused tab now lists each new packet once, and repeated reloads no longer add subscribers or a database read per live packet. Closed network tabs stop receiving packets too, while open tabs, including .etl imports, keep receiving theirs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 270d0c0c-8578-4bd6-aa1d-c32a56075710
23c7379 to
3357889
Compare
A network capture that is being loaded no longer listens for live packets until it becomes a tab, so a packet that arrives while a capture is reloaded into an existing tab is listed once instead of twice. While a network capture is running, a loaded tab for the same device shows the new packets from when it opens or reloads; packets that arrive while the file loads still appear in the running capture's tab and are saved. Retrying a network capture after a failed start no longer lists its packets twice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 270d0c0c-8578-4bd6-aa1d-c32a56075710
Validate capture-file metadata and improve import failure handling. Includes compatibility and cleanup tests.