Martino FerrariandClaude Sonnet 4.6
ffec797462
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>
101 lines
5.5 KiB
Markdown
101 lines
5.5 KiB
Markdown
# 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.
|