[codex] Sanitize discovery padding bytes#3519
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the SmartSDR local discovery parser by sanitizing parsed key=value fields to remove trailing ASCII control/padding bytes (e.g., NUL/CR/DEL) before values are stored in RadioInfo, preventing control-glyph artifacts in the connection dialog UI.
Changes:
- Added centralized discovery-value sanitization (
cleanDiscoveryValue) and list parsing (splitDiscoveryListValue) inRadioDiscovery::parseDiscoveryPacket. - Preserved the legacy DEL-as-space decoding behavior for GUI-client list fields before sanitizing/splitting.
- Introduced a focused regression test (
radio_discovery_test) and wired it into CMake/CTest.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/radio_discovery_test.cpp | Adds a regression test ensuring padding/control bytes are stripped and DEL-separated GUI client fields still decode properly. |
| src/core/RadioDiscovery.h | Exposes the packet parser to tests via a guarded friend declaration under AETHERSDR_TESTING. |
| src/core/RadioDiscovery.cpp | Implements value sanitization and uses it across scalar and list discovery fields. |
| CMakeLists.txt | Adds and registers the new radio_discovery_test executable, enabling the test-only friend access. |
jensenpat
left a comment
There was a problem hiding this comment.
Reviewed: sanitization at the parser boundary is correct, DEL-as-space convention preserved for gui_client_* fields, regression test covers the reported padding symptom. CI green.
There was a problem hiding this comment.
Thanks @rfoust — this is a well-scoped fix in the right place. Sanitizing at the parser boundary (rather than in ConnectionPanel) means every consumer of RadioInfo benefits, the DEL-as-space convention for the gui_client list fields is correctly preserved by decoding before stripping, and the regression test plus the standalone test target follow the existing pattern (client_quindar_test et al.). I verified the root cause holds: QString::trimmed() doesn't treat NUL as whitespace, so turf_region=USA\0\0\r previously survived as USA\0\0. The test's lcDiscovery definition also correctly lands inside namespace AetherSDR, matching the Q_DECLARE_LOGGING_CATEGORY in LogManager.h, so it links cleanly without pulling in LogManager.cpp. All four changed files are within the stated scope.
A few minor observations, none blocking:
-
Bytes ≥ 0x80 survive the filter. The packet is decoded with
QString::fromUtf8()before sanitizing, so non-UTF-8 padding bytes like0xFFbecome U+FFFD replacement characters, which pass thecode < 0x20 || code == 0x7fcheck and would still render as placeholder glyphs. If you want to harden this fully, also skipQChar::ReplacementCharacter(0xFFFD) incleanDiscoveryValue(). The reported symptom was NUL/CR padding, so this is optional. -
Parallel-list alignment (edge case, worth knowing). The
gui_client_*lists are consumed as parallel arrays downstream (RadioModel::setKnownGuiClients,MainWindowHelpers::buildDisconnectClientspair handles[i] with stations[i]/ips[i]). The new per-item trim-and-drop means an entry that's only control characters now disappears from one list, which could shift indices relative to its siblings. The old code had the mirror-image quirk (garbage placeholder entries), and real radios pad at packet end rather than mid-list, so this is fine as-is — just flagging it since the alignment contract isn't obvious from the parser. -
Copilot's include note is valid but minor. The test uses
QByteArrayandQStringListvia transitive includes fromRadioDiscovery.h/QCoreApplication. Adding the two explicit includes is cheap insurance against future Qt header reshuffles — worth doing if you push another revision, not worth one on its own. -
AETHERSDR_TESTING+ friend class is a new pattern in this codebase (existing tests exercise public APIs). It's a reasonable, contained way to reach the private parser, and the#ifdefkeeps it out of production builds — no objection, just noting it's precedent-setting if other tests copy it.
Nice catch on the screenshot symptom, and thanks for the focused regression test.
🤖 aethersdr-agent · cost: $10.3019 · model: claude-fable-5
Root Cause
Local SmartSDR discovery packets are decoded as UTF-8 and parsed as space-separated
key=valuetokens, but the parser previously stored most values verbatim. If a discovery packet carried NUL, carriage-return, or other control/padding bytes after a printable value such asturf_region=USA, those bytes survivedQString::trimmed()when embedded in the token. The connection dialog then appendedradio.turfRegion.trimmed()directly to the available-radio label, so Qt rendered the control bytes as visible placeholder glyphs afterUSA.User Impact
The available-radio list could show stray box/control glyphs after otherwise valid discovery metadata. The screenshot symptom was
USAfollowed by several placeholder characters in the local radio list subtitle.Change Summary
RadioDiscoveryparser boundary.RadioInfo.gui_client_stations,gui_client_programs, andgui_client_hostsbefore sanitizing and splitting those list values.radio_discovery_testregression case that feeds a padded discovery packet and verifiesturf_regionis clean while DEL-encoded GUI client fields still decode correctly.Validation
./build/radio_discovery_testctest --test-dir build -R radio_discovery_test --output-on-failuregit diff --checkcmake --build build --target AetherSDR -j4