Skip to content

Fix premature CAN map limit (~20 signals) caused by off-by-one bounds check aliasing send/recv maps#48

Merged
jsphuebner merged 2 commits intomasterfrom
copilot/fix-canvas-mapping-bug
Apr 27, 2026
Merged

Fix premature CAN map limit (~20 signals) caused by off-by-one bounds check aliasing send/recv maps#48
jsphuebner merged 2 commits intomasterfrom
copilot/fix-canvas-mapping-bug

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 27, 2026

Users hit SDO_ERR_INVIDX at subindex 2 (~20 TX signals) well below the documented limits (50 items / 10 messages) because GetMap and Remove used > instead of >= when bounds-checking the message index against MAX_MESSAGES.

Primary bug: send/recv map aliasing (canmap.cpp)

canSendMap and canRecvMap are adjacent arrays. Index MAX_MESSAGES (=10) on the send map silently aliases canRecvMap[0]:

  • GetMap(false, 10, ...) returned recv map data as a valid TX entry — tools enumerating send messages saw a phantom 11th entry
  • Remove(false, 10, 0) deleted canRecvMap[0] while the caller believed it was removing a TX entry
  • In Remove's case-3 path, lastIdx is uint8_t and (0 + 10) < 10 is immediately false, so lastIdx-- underflows to 255 — a massive out-of-bounds write corrupting adjacent memory

The combination consumes send map slots illegitimately, causing CAN_ERR_MAXMESSAGES far below the actual limit.

// Before (both GetMap and Remove):
if (ididx > MAX_MESSAGES || ...)   // allows index == MAX_MESSAGES

// After:
if (ididx >= MAX_MESSAGES || ...)  // correct upper bound

Secondary bugs: CanSdo initialization (cansdo.cpp)

  • mapId initialized to 0 instead of 0xFFFFFFFF — subindex-1 handler accepted a write without a prior valid subindex-0 (CAN ID)
  • mapInfo left uninitialized — garbage numBits != 0 could trigger a spurious AddSend/AddRecv on the first subindex-2 SDO after boot
  • uint8_t underflow in ReadOrDeleteCanMap: MAX(0, sdo->subIndex - 1) / 2 wraps to 127 when subIndex == 0, making all reads/deletes of the CAN-ID sub-object fail with SDO_ABORT
// Before:
uint8_t itemIdx = MAX(0, sdo->subIndex - 1) / 2;  // wraps when subIndex==0

// After:
uint8_t itemIdx = sdo->subIndex == 0 ? 0 : (sdo->subIndex - 1) / 2;

Tests

Two regression tests added to test_canmap.cpp:

  • get_map_at_max_messages_returns_null — verifies GetMap(false, MAX_MESSAGES, ...) returns null, not recv map data
  • remove_at_max_messages_is_safe — verifies Remove(false, MAX_MESSAGES, 0) is a no-op and leaves canRecvMap[0] intact

Copilot AI linked an issue Apr 27, 2026 that may be closed by this pull request
…ubIndex underflow

Agent-Logs-Url: https://github.com/jsphuebner/libopeninv/sessions/16913bdb-6be2-4d7d-80c0-c4aa1253fac0

Co-authored-by: jsphuebner <3882041+jsphuebner@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix bug limiting CANMap for transmit messages Fix premature CAN map limit (~20 signals) caused by off-by-one bounds check aliasing send/recv maps Apr 27, 2026
Copilot AI requested a review from jsphuebner April 27, 2026 09:13
@davefiddes
Copy link
Copy Markdown
Contributor

Changes to CanMap look correct to me.

Lack of tests for CanSdo isn't great. I never felt strong enough to write them.

@jsphuebner jsphuebner marked this pull request as ready for review April 27, 2026 09:48
@jsphuebner jsphuebner merged commit aaa1a22 into master Apr 27, 2026
2 checks passed
@jsphuebner
Copy link
Copy Markdown
Owner

Thanks for reviewing, Dave.
Will let Copilot draft some tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug limiting CANMap

3 participants