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>
This commit is contained in:
1 parent
b8d0ce8625
commit
ffec797462
6 files changed
+151
-16
No files matched your search
@@ -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 (`<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.
|
||||
Reference in new issue
Block a user