Files
udpscope/.superpowers/sdd/final-fixes-report.md
T
Martino FerrariandClaude Sonnet 4.6 ffec797462 fix code-review findings in the source-chooser feature
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>
2026-10-04 10:03:48 +02:00

5.5 KiB
Raw Blame History

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.