From ffec7974622a70d068efa283b37c1cd49fb73dc8 Mon Sep 17 00:00:00 2001 From: Martino Ferrari Date: Sun, 4 Oct 2026 10:03:48 +0200 Subject: [PATCH] fix code-review findings in the source-chooser feature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ( before ). - 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 --- .superpowers/sdd/final-fixes-report.md | 100 +++++++++++++++++++++++++ CLAUDE.md | 10 ++- README.md | 5 +- src/layout_store.cpp | 19 +++-- src/source_dialog.cpp | 27 +++++-- src/ui.cpp | 6 ++ 6 files changed, 151 insertions(+), 16 deletions(-) create mode 100644 .superpowers/sdd/final-fixes-report.md diff --git a/.superpowers/sdd/final-fixes-report.md b/.superpowers/sdd/final-fixes-report.md new file mode 100644 index 0000000..eb65ae9 --- /dev/null +++ b/.superpowers/sdd/final-fixes-report.md @@ -0,0 +1,100 @@ +# 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 (`` before `` — 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 `` and `` 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. diff --git a/CLAUDE.md b/CLAUDE.md index abe516a..78e5aac 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -462,8 +462,8 @@ Two lines of the self-check output are assertions in disguise: networking**: write the session file by hand and read the line. With `UDPSCOPE_PROBE_NEWHOST=host:port` it drives the form instead — `[sources] form open, connect_sensitive=1` then a `picked` line, with no SIGSEGV, which is as much a crash test of a - dialog built and destroyed as it is a state check. An empty list must print `picked nothing` - rather than dereferencing a null row. + dialog built and destroyed as it is a state check. An empty list must print `picked nothing: + the list is empty` rather than dereferencing a null row. - **The session layout round trip** (`~/.config/udpscope/_.toml`) — run once with a drop and a window change, then again with no interaction: the second run's dump must read `layout loaded from ...` and `layout: rows=R window_s=W row0=[...]` with the @@ -1273,4 +1273,10 @@ far the largest file. least one signal**, because a probe run leaves a file per throwaway port and all of them hold `signals = ["", "", "", ""]`; `known_sources()` splits the filename at the **last** `_`, since `sanitize()` turns anything unsafe in a host into one. +- **`save_layout_now()` refuses to write while `signals()` is empty.** A run against a source + whose streamer was down receives no CONFIG, so every slot name stays empty and the save would + overwrite a configured session with `signals = ["", ...]`. The chooser then filters that file + out — a source whose streamer was merely down during one run would silently vanish from the + only list it can be picked from, for ever. Keeping the old file loses nothing: a run that + configured nothing had nothing worth persisting. - Every source file carries an SPDX MIT header; match it in new files. diff --git a/README.md b/README.md index 21a056d..f4c6af7 100644 --- a/README.md +++ b/README.md @@ -480,7 +480,10 @@ once already. And the list is **filtered to sessions that name a signal**: a dev accumulates one file per probe run on a throwaway port — 140 of them here, every one with `signals = ["", "", "", ""]` — so an unfiltered chooser opens as a wall of ports nobody chose. A row's `×` forgets a source by deleting its session file, with no confirmation: the gesture is the -confirmation, and what it destroys is one layout the app rewrites on the next clean exit. +confirmation, and what it destroys is one layout the app rewrites on the next clean exit. A session +is **not written** when no CONFIG arrived during the run — otherwise a source whose streamer was +down during one launch would silently vanish from the chooser list because its file was flattened +to empty signal names, which the filter refuses. **Math channels.** A math channel is derived from the others by a graph of operations and is then an *ordinary channel*: the model synthesises a signal entry for it, so an enabled one is acquired, diff --git a/src/layout_store.cpp b/src/layout_store.cpp index d7eecd1..eecb632 100644 --- a/src/layout_store.cpp +++ b/src/layout_store.cpp @@ -5,8 +5,8 @@ #include #include -#include #include +#include #include #include #include @@ -134,15 +134,22 @@ std::vector known_sources() { // may hold them too and a first-match split would return a host cut in half. const std::size_t cut = stem.rfind('_'); if (cut == std::string::npos || cut + 1 == stem.size()) continue; - const long port = std::strtol(stem.c_str() + cut + 1, nullptr, 10); - if (port <= 0 || port > 65535) continue; + char* end = nullptr; + const long port = std::strtol(stem.c_str() + cut + 1, &end, 10); + if (end == nullptr || *end != '\0' || port <= 0 || port > 65535) continue; + const std::string host = stem.substr(0, cut); + // The name must be one `layout_path()` could have written, or the `×` -- which rebuilds it + // through `layout_path()` -- would unlink a different session. `sanitize()` is idempotent on + // any host that already came back out of a filename, so this is also true for hosts whose names + // contain `_` from the sanitizer. + if (sanitize(host) + "_" + std::to_string(port) != stem) continue; LayoutState st; const std::string path = dir + "/" + name; if (!load_layout(path, st)) continue; KnownSource s; - s.host = stem.substr(0, cut); + s.host = host; s.port = static_cast(port); s.multicast = st.multicast; s.interface_addr = st.interface_addr; @@ -385,7 +392,9 @@ bool save_layout(const std::string& path, const LayoutState& state) { o << "# udpscope session layout (written by the app; safe to edit)\n"; if (state.window_s > 0.0) o << "window_s = " << state.window_s << "\n"; o << "plots = " << state.rows.size() << "\n"; - // Top level, before any table, for the same reason as `enabled` below. + // Top level, before any table. A bare key written after a `[[table]]` header belongs to that + // table in TOML, which is exactly how `enabled` was once parsed into the last `[[calibration]]` + // record and silently lost. if (!state.multicast.empty()) o << "multicast = " << toml_escape(state.multicast) << "\n"; if (!state.interface_addr.empty()) { o << "interface = " << toml_escape(state.interface_addr) << "\n"; diff --git a/src/source_dialog.cpp b/src/source_dialog.cpp index 8fd34cb..5fa8505 100644 --- a/src/source_dialog.cpp +++ b/src/source_dialog.cpp @@ -64,6 +64,16 @@ struct Chooser { void chooser_finish(Chooser& c, bool chosen) { c.chosen = chosen; + // Destroy the form first: any path that reaches here while the form is up -- a row click, the + // Escape key -- would otherwise leave a window whose four handlers are bound to a dead stack + // frame. `gtk_window_destroy()` rather than `gtk_window_close()`: the pointer is already + // cleared, so the `close-request` handler is not needed here, and `close()` on an unrealized + // window returns having done nothing. + if (c.form != nullptr) { + GtkWidget* w = c.form; + c.form = nullptr; // before the destroy, so the `close-request` handler cannot re-enter + gtk_window_destroy(GTK_WINDOW(w)); + } if (c.window != nullptr) { GtkWidget* w = c.window; c.window = nullptr; // before the destroy: `close-request` must not re-enter this @@ -104,7 +114,9 @@ void chooser_populate(Chooser& c) { gtk_widget_add_css_class(empty, "dim-label"); gtk_widget_set_margin_top(empty, 24); gtk_widget_set_margin_bottom(empty, 24); - // Not a `GtkListBoxRow`, so it cannot be activated into a source that does not exist. + // GTK4 auto-wraps any non-row widget appended to a `GtkListBox` in a row, so this is + // activatable -- but the wrapped row carries no `"source-index"` data, `g_object_get_data()` + // returns null, the index reads 0, and `on_row_activated`'s bounds check refuses it. gtk_list_box_append(GTK_LIST_BOX(c.list), empty); gtk_list_box_set_selection_mode(GTK_LIST_BOX(c.list), GTK_SELECTION_NONE); return; @@ -310,12 +322,6 @@ gboolean pick_probe(gpointer user) { return G_SOURCE_REMOVE; } GtkListBoxRow* row = gtk_list_box_get_row_at_index(GTK_LIST_BOX(c.list), 0); - if (row == nullptr) { - std::fprintf(stderr, "[sources] picked nothing: the list is empty\n"); - std::fflush(stderr); - chooser_finish(c, false); - return G_SOURCE_REMOVE; - } // `row-activated`, not `gtk_widget_activate()`: this project has already learned that // activating a widget takes a path a real click does not (a `GtkButton` holds for its 250 ms // ACTIVATE_TIMEOUT and then emits into a widget a rebuild has destroyed). @@ -388,14 +394,19 @@ bool choose_source(Config& cfg) { gtk_window_present(GTK_WINDOW(c.window)); + // Keep the timeout id so it can be cancelled if the chooser finishes before it fires. A row + // clicked before UDPSCOPE_PROBE_PICK_MS elapses would otherwise leave an armed source on the + // default main context pointing at the now-gone stack frame. + guint pick_probe_id = 0; if (const char* env = g_getenv("UDPSCOPE_PROBE_PICK_MS")) { - g_timeout_add(static_cast(std::atoi(env)), pick_probe, &c); + pick_probe_id = g_timeout_add(static_cast(std::atoi(env)), pick_probe, &c); } c.loop = g_main_loop_new(nullptr, FALSE); g_main_loop_run(c.loop); g_main_loop_unref(c.loop); c.loop = nullptr; + if (pick_probe_id != 0) g_source_remove(pick_probe_id); return c.chosen; } diff --git a/src/ui.cpp b/src/ui.cpp index 22331f5..2e8ab52 100644 --- a/src/ui.cpp +++ b/src/ui.cpp @@ -573,6 +573,12 @@ LayoutState collect_layout(App& app) { /// stderr; success is silent -- a save per click must not narrate. void save_layout_now(App& app) { if (app.source_port == 0) return; + // Nothing is known about this source yet -- no CONFIG ever arrived -- so every slot name is + // empty and this would overwrite a configured session with `signals = ["", ...]`. The startup + // chooser filters such a file out of its list, so a source whose streamer was down during one + // run would silently vanish from the only place it can be picked. Keeping the old file loses + // nothing: this run configured nothing. + if (app.model->signals().empty()) return; if (!save_layout(layout_path(app.source_host, app.source_port), collect_layout(app))) { std::fprintf(stderr, "[udpscope] failed to write the layout file\n"); std::fflush(stderr);