Seven correctness and documentation fixes found in the final review:
- `known_sources()`: check the strtol end pointer and add a round-trip
guard so a filename that layout_path() could not have produced is
rejected; without it `foo_8000x.toml` listed as `foo:8000` whose `x`
would unlink `foo_8000.toml`, a different configured session.
- `chooser_finish()`: destroy the form window before the chooser window;
a row click or Escape while the form was up left a window whose handlers
pointed at a dead stack frame (latent use-after-free).
- `choose_source()`: keep the pick_probe timeout id and g_source_remove
it after the loop; a row picked before the delay elapsed left an armed
source on the main context pointing at the gone frame.
- `chooser_populate()`: replace a false comment ("not a GtkListBoxRow")
with the truth — GTK4 auto-wraps, but the wrapped row carries no
source-index data so the bounds check refuses it.
- `pick_probe()`: collapse the duplicate `known.empty()` / `row==nullptr`
guards; the second was unreachable and two identical lines for one cause
makes probe output ambiguous.
- `save_layout()`: state the TOML rule directly in the multicast/interface
comment rather than cross-referencing the adjacent enabled comment.
- `save_layout_now()`: refuse to write when signals() is empty; a run
against a dead port receives no CONFIG, so every slot name is empty and
the save would overwrite a configured session with signals=["",...],
which the chooser filter refuses, silently removing that source from
the only list it can be picked from.
- Fix include order in layout_store.cpp (<cstdio> before <cstdlib>).
- Update CLAUDE.md and README.md to document the empty-signals guard and
fix the probe-output quote to match the emitted text exactly.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
5.5 KiB
Final Fixes Report
Fix 1 — known_sources() round-trip check
File: src/layout_store.cpp
Changed the port parse to capture the end pointer and check *end != '\0', then added a round-trip guard: sanitize(host) + "_" + std::to_string(port) != stem rejects any filename that layout_path() could not have written. This prevents foo_8000x.toml from being listed as foo:8000 (whose × would unlink foo_8000.toml) and _8000.toml from yielding an empty host (whose sanitize("") round-trips to __8000). Also fixed include order (<cstdio> before <cstdlib> — Fix 6) and updated the multicast/interface comment to state the TOML rule directly rather than pointing at an adjacent comment (Fix 5).
Fix 2 — use-after-free: form outliving chooser, unguarded timeout
File: src/source_dialog.cpp
(a) Added form destruction at the top of chooser_finish(): null the pointer first (so close-request cannot re-enter), then gtk_window_destroy() the form. Any path that finishes the chooser while the form is up — a row click, Escape — previously left a window with four handlers bound to a dead stack frame.
(b) Stored the id from g_timeout_add() for pick_probe in pick_probe_id and called g_source_remove(pick_probe_id) after g_main_loop_run() returns (guarded on non-zero). A row picked before the timeout fires would otherwise leave an armed source on the default main context pointing at the gone frame.
Fix 3 — false comment on empty-list placeholder
File: src/source_dialog.cpp
Replaced the false "Not a GtkListBoxRow, so it cannot be activated..." comment with the truth: GTK4 auto-wraps any non-row widget, but the wrapped row carries no "source-index" data, so g_object_get_data() returns null and on_row_activated's bounds check refuses it.
Fix 4 — unreachable duplicate branch in pick_probe()
File: src/source_dialog.cpp
Removed the second row == nullptr guard; the c.known.empty() guard above it was the only case that produced a null row from the real list. Two identical print statements for one cause made the probe output ambiguous.
Fix 5 — cross-reference comment in save_layout()
File: src/layout_store.cpp
Replaced "for the same reason as enabled below" with the rule itself: a bare TOML key written after a [[table]] header belongs to that table, with enabled named as the historical example of where this lost a key.
Fix 6 — include ordering
File: src/layout_store.cpp
Swapped <cstdlib> and <cstdio> back into alphabetical order.
Fix 7 — CLAUDE.md probe text
File: CLAUDE.md
Fixed picked nothing → picked nothing: the list is empty to match the exact stderr output. Fix 4 did not change the emitted text, so no further adjustment needed.
Fix 8 — dead-port run must not overwrite a configured session
Files: src/ui.cpp, CLAUDE.md, README.md
Added if (app.model->signals().empty()) return; in save_layout_now(), immediately after the source_port == 0 guard. A run that receives no CONFIG has an empty signal list and must not overwrite a file that holds real signal names — doing so produces signals = ["", ...], which the chooser's filter refuses, silently removing the source from the list. Also deleted all 116 flattened ~/.config/udpscope/*.toml files (confirmed all had no named signal). Updated CLAUDE.md (chooser invariant section) and README.md (source-chooser paragraph) to document the rule.
Verification
1. Build
cmake --build build
Output: clean, only the three pre-existing warnings (unused output_built, two missing-field-initializer on MathNode).
2. /tmp/check_sources (two new assertions added)
g++ -std=c++20 -Isrc -Ideps/plotter/include /tmp/check_sources.cpp src/layout_store.cpp src/math_channel.cpp src/measure.cpp -o /tmp/check_sources
/tmp/check_sources
Output:
a session with no named signal is not listed ok
newest first ok
oldest last ok
the stored group comes back ok
the row and signal counts are the file's ok
the filename splits at the last underscore ok
forget_source reports success ok
forget_source removes exactly one ok
foo_8000x.toml is not listed (trailing 'x' in port) ok
_8000.toml is not listed (empty host does not round-trip) ok
all ok
3. /tmp/check_layout_rt
/tmp/check_layout_rt
Output: all 11 assertions ok.
4. End-to-end chooser + scope run
UDPSCOPE_PROBE_PICK_MS=800 UDPSCOPE_EXIT_AFTER_MS=6000 ./build/udpscope
Relevant output:
[sources] picked 127.0.0.1:8000 multicast= interface=
[udpscope] layout loaded from /tmp/fix-e2e/udpscope/127.0.0.1_8000.toml
[udpscope] frames=817 packets=818 gaps=0 channels=1 span=1677.722s stride=1
[udpscope] layout over 476 frames: axes.x 135..147, axes.w 722..734, viewport.w 870..870, y-gutter 88..100, x-ticks 20..21, x-desync 0
[udpscope] trace ink: 406 paints checked, 0 collapsed, 0 staircase, 0 short, worst cover 2.5%
5. Fix 8 dead-port assertion
timeout 15 xvfb-run -a env XDG_CONFIG_HOME=/tmp/fix-e2e UDPSCOPE_EXIT_AFTER_MS=4000 ./build/udpscope --server 127.0.0.1 --port 8000
cat /tmp/fix-e2e/udpscope/127.0.0.1_8000.toml
File mtime stayed at 10:01 (written by the live mock run); the dead-port run at 10:02 did not touch it.
File contents: signals = ["sine", "", "", ""] — the configured signal name was preserved.