Fix correctness bugs in signal decoding and frame sending#166
Open
sebi2k1 wants to merge 2 commits into
Open
Conversation
Four small, surgical fixes for silent-data-corruption and UB bugs identified in the full code review: - socketcan.ts: replace `x << 32` (which is `x << 0` in JS) with `x * TWO_TO_32` when reassembling the high word of decoded signals. The old expression silently dropped the high 32 bits for any signal wider than 32 bits. - socketcan.ts: switch `min/maxValue` bounds checks from truthy to `!= null` so that a legitimate bound of `0` is honoured. - can.cc: reject Send() payloads larger than CAN_MAX_DLEN before the memcpy into the 8-byte `struct can_frame.data`, preventing a stack overwrite on caller error. - signals.cc: validate that `bitLength` is in the range 1..64 in both DecodeSignal and EncodeSignal. Out-of-range values previously caused shifts of full type width / by negative amounts (UB). Tests: - test-signal_conversion.js: high-word regression case plus bitLength range validation for encode and decode. - test-signal_bounds.js: new file covering the `0`-boundary cases for Signal min/maxValue. - test-send_overflow.js: new file asserting Send() throws on oversized buffers (skips when vcan0 is unavailable). Also includes ESLint auto-fix formatting touched by `npm run lint` (trailing commas, switch reformatting) across `parse_kcd.ts`, `can.d.ts`, `can_signals.d.ts`, and `socketcan.ts`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
4 tasks
The previous _getvalue / _setvalue always memcpy'd bytes 0..7 of the local buffer into a uint64_t and shifted from there. That assumption predates CAN-FD support: for any signal whose bit offset lies past byte 7 of a 64-byte CAN-FD payload, the routine returned garbage or hit shift-by-full-type-width UB (Intel offset >= 64) / size_t underflow on the Motorola shift formula 64 - offset - length (offset > 64). Encoding had the symmetric bug. Rewrite both helpers as a windowed fast path plus a per-bit fallback: - Fast path (essentially all real signals): memcpy 8 bytes starting at data[offset/8], endian-swap, then shift by (offset % 8) for Intel or (64 - bit_in_byte - length) for Motorola, then mask. Same shape and cost as the previous code, just with the byte_start factored out. - Fallback: when bit_in_byte + length > 64 — a 58..64 bit signal at a non-byte-aligned offset, essentially never seen in real CAN databases — walk the signal bit by bit. Both endiannesses handled. The local scratch buffer grows from 64 to 72 bytes (MAX_PAYLOAD_BYTES + WINDOW_SAFETY_PAD = 8) so the 8-byte windowed memcpy is always in-bounds even for a signal sitting at byte 63. The padding stays zero and any bits read from it are masked off. EncodeSignal now also memsets the local buffer before the memcpy-in. The fast-path RMW may touch bytes past the caller's payload inside the scratch buffer, and previously those bytes were uninitialised stack memory — harmless because they're never written back, but flagged as UB by the model. Two new bounds checks are added in DecodeSignal and EncodeSignal: - offset + effectiveBitLength must fit in MAX_TOTAL_BITS (512). - signal must fit within the caller's JS buffer length. Both throw TypeError with a clear message rather than silently producing wrong results. The KAYAK_DATA_CHECK debug blocks are removed: the per-bit walk that they referenced is now the production fallback path, so the self-comparison is no longer meaningful. Tests cover all the worked examples plus the boundaries: - Intel 16-bit at offset 200 (decode + encode, RMW preserves neighbours) - Motorola 12-bit at byte-aligned offset 144 (decode + encode) - Motorola 10-bit at non-byte-aligned offset 147 (round-trip) - 8-bit at byte 63 (last byte of CAN-FD payload) - 64-bit Intel at offsets 0 and 64 (length-64 boundary) - 58-bit Intel at offset 7 (exercises the per-bit fallback) - All four out-of-bounds throws (frame and buffer). All 13 new cases plus the 27 pre-existing signal tests pass in the node:22-bookworm-slim Docker container. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four small, surgical fixes for silent-data-corruption and UB bugs surfaced by a full code-review pass:
socketcan.ts(× 2 sites) — replacex << 32(a no-op on JS 32-bit shift semantics) withx * TWO_TO_32when reassembling decoded high words. Previously, any signal wider than 32 bits silently lost its top half.socketcan.ts(× 2 sites) — changeSignal.update()min/max guards from truthy (this.maxValue && ...) to!= null, so a legitimate bound of0is enforced instead of skipped.native/can.cc— inRawChannel::Send, reject buffers larger thanCAN_MAX_DLENbefore thememcpyintostruct can_frame.data(8 bytes). Prevents a stack overwrite on caller error.SendFDalready clamps to 64 and is unchanged.native/signals.cc— validatebitLengthis in1..64in bothDecodeSignalandEncodeSignal. Out-of-range values previously produced shifts of full type width or by underfloweduint32_t(undefined behavior).The PR also includes ESLint auto-fix formatting (
npm run lint) acrossparse_kcd.ts,can.d.ts,can_signals.d.ts, and parts ofsocketcan.ts— trailing commas, switch-statement reformatting, onelet→const. No behaviour changes in those hunks.Items #3 (
id | (1<<31)brittleness) and #4 (CAN-FD bit-extraction window) from the same review are out of scope here — #3 is being demoted from "correctness" and #4 is being tracked as its own larger PR.Test plan
docker build -f Dockerfile.build-test -t node-can-build-test .— native + TS build passes (was validated asnpm run build:tslocally; Docker not available on review machine).npm run lint— clean.npm teston Linux withvcan0/vcan1:test-signal_conversion.js— 5 new cases pass (high-word regression, bitLength range × 4).test-signal_bounds.js— 5 cases pass coveringmin/maxValue: 0boundaries.test-send_overflow.js— passes against realvcan0; skips cleanly if absent.🤖 Generated with Claude Code