output_xml: add SAMPLE_EVENT_STRING to events table and guard array access - #135
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new use of value->event.name is written into XML without escaping, which can produce malformed XML when the name contains characters like & or <.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes an out-of-bounds access in the XML sample event string table by extending the table for the newly added SAMPLE_EVENT_STRING type and by guarding event-table indexing to avoid UB for future enum additions. It also improves XML output semantics by using value->event.name as the label for SAMPLE_EVENT_STRING when available.
Changes:
- Extend the
events[]lookup table to include index 26 forSAMPLE_EVENT_STRING. - Add bounds checking before indexing
events[], falling back to"unknown"for out-of-range event types. - Prefer
value->event.nameforSAMPLE_EVENT_STRINGevents when non-NULL.
File summaries
| File | Description |
|---|---|
| examples/output_xml.c | Fixes event string table OOB, guards future indexing, and emits dynamic names for string events. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ccess SAMPLE_EVENT_STRING (value 26) was added to parser_sample_event_t after SAMPLE_EVENT_GASCHANGE2 (value 25), but the events[] string table in sample_cb() had only 26 entries (indices 0-25). Accessing events[26] is undefined behaviour and aborts under UBSan. Three changes: - Append "string" at index 26 so the table covers all currently defined event types. - Add a bounds guard (sizeof(events)/sizeof(events[0])) before indexing events[]. Out-of-range types fall back to "unknown" so future additions do not crash before the table is updated. - When the event type is SAMPLE_EVENT_STRING and value->event.name is non-NULL, use the dynamic name string rather than the static "string" label. This preserves the semantic intent of the type (free-form annotations such as compass heading and scrubber state from hw_ostc and Garmin parsers). Reproduces with: hw_ostc5-0001.bin, garmin_descent_mk1-0001.bin. Signed-off-by: Michael Keller <github@ike.ch>
405f98f to
2aea4df
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new XML escaping helper uses fputc(*s, ...) without unsigned-char casting, which can trigger undefined behavior for non-ASCII/UTF-8 bytes, and there’s an unnecessary “AI-generated” attribution comment to clean up.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
fputc() requires its int argument to be representable as unsigned char or EOF (C17 §7.21.7.3p2). Passing a plain char directly is UB on signed-char platforms when the value is > 127. Cast *s to (unsigned char) at the call site to make the argument well-defined on all platforms. Signed-off-by: Michael Keller <github@ike.ch>
SAMPLE_EVENT_STRING (value 26) was added to parser_sample_event_t after
SAMPLE_EVENT_GASCHANGE2 (value 25), but the events[] string table in
sample_cb() had only 26 entries (indices 0-25). Accessing events[26]
is undefined behaviour and aborts under UBSan.
Three changes:
event types.
events[]. Out-of-range types fall back to "unknown" so future additions
do not crash before the table is updated.
non-NULL, use the dynamic name string rather than the static "string"
label. This preserves the semantic intent of the type (free-form
annotations such as compass heading and scrubber state from hw_ostc
and Garmin parsers).
Reproduces with: hw_ostc5-0001.bin, garmin_descent_mk1-0001.bin.