diff --git a/src/canmap.cpp b/src/canmap.cpp index 85e6530..18b4606 100644 --- a/src/canmap.cpp +++ b/src/canmap.cpp @@ -360,7 +360,7 @@ int CanMap::Remove(bool rx, uint8_t messageIdx, uint8_t itemidx) CANPOS *lastPosMap = 0; CANIDMAP *map = rx ? &canRecvMap[messageIdx] : &canSendMap[messageIdx]; - if (messageIdx > MAX_MESSAGES || map->first == MAX_ITEMS) return 0; + if (messageIdx >= MAX_MESSAGES || map->first == MAX_ITEMS) return 0; forEachPosMap(curPos, map) { @@ -487,7 +487,7 @@ const CanMap::CANPOS* CanMap::GetMap(bool rx, uint8_t ididx, uint8_t itemidx, ui { CANIDMAP *map = rx ? &canRecvMap[ididx] : &canSendMap[ididx]; - if (ididx > MAX_MESSAGES || map->first == MAX_ITEMS) return 0; + if (ididx >= MAX_MESSAGES || map->first == MAX_ITEMS) return 0; forEachPosMap(curPos, map) { diff --git a/src/cansdo.cpp b/src/cansdo.cpp index 8873ffa..d10b21e 100644 --- a/src/cansdo.cpp +++ b/src/cansdo.cpp @@ -47,7 +47,7 @@ CanSdo::CanSdo(CanHardware* hw, CanMap* cm) : canHardware(hw), canMap(cm), nodeId(1), remoteNodeId(255), printRequest(-1), printByteIn(0), printByteOut(sizeof(printBuffer)), printTimeout(PRINT_TIMEOUT), - mapParam(Param::PARAM_INVALID), mapId(0), sdoReplyValid(false), sdoReplyData(0), + mapParam(Param::PARAM_INVALID), mapId(0xFFFFFFFF), mapInfo{}, sdoReplyValid(false), sdoReplyData(0), pendingUserSpaceSdo(false) { canHardware->AddCallback(this); @@ -304,7 +304,7 @@ void CanSdo::ReadOrDeleteCanMap(SdoFrame* sdo) { bool rx = (sdo->index & 0x80) != 0; uint32_t canId; - uint8_t itemIdx = MAX(0, sdo->subIndex - 1) / 2; + uint8_t itemIdx = sdo->subIndex == 0 ? 0 : (sdo->subIndex - 1) / 2; const CanMap::CANPOS* canPos = canMap->GetMap(rx, sdo->index & 0x3f, itemIdx, canId); if (sdo->cmd == SDO_READ) diff --git a/test/test_canmap.cpp b/test/test_canmap.cpp index a0a4246..183b429 100644 --- a/test/test_canmap.cpp +++ b/test/test_canmap.cpp @@ -581,6 +581,36 @@ static void fail_to_map_with_invalid_big_endian_total_struct_offset() } +// Regression test: GetMap and Remove must not access out-of-bounds send map entry +// at ididx/messageIdx == MAX_MESSAGES, which would alias canRecvMap[0] +static void get_map_at_max_messages_returns_null() +{ + // Add a recv mapping so canRecvMap[0] has data + canMap->AddRecv(Param::ocurlim, 0x300, 0, 8, 1.0, 0); + + uint32_t canId = 0; + // Querying tx map at index MAX_MESSAGES must return null (not canRecvMap[0]'s data) + const CanMap::CANPOS* pos = canMap->GetMap(false, MAX_MESSAGES, 0, canId); + ASSERT(pos == 0); +} + +// Regression test: Remove with messageIdx == MAX_MESSAGES must not delete canRecvMap[0] +static void remove_at_max_messages_is_safe() +{ + // Add a recv mapping so canRecvMap[0] has data + canMap->AddRecv(Param::ocurlim, 0x300, 0, 8, 1.0, 0); + + // Attempting to remove a tx message at index MAX_MESSAGES must be a no-op + int removed = canMap->Remove(false, MAX_MESSAGES, 0); + ASSERT(removed == 0); + + // The recv mapping must still be intact + uint32_t canId = 0; + const CanMap::CANPOS* pos = canMap->GetMap(true, 0, 0, canId); + ASSERT(pos != 0); + ASSERT(canId == 0x300); +} + static void create_and_delete_complex_map_once() { canMap->AddSend(Param::amp, 257, 24, 8, -1.00, 0); @@ -1172,4 +1202,6 @@ REGISTER_TEST( fail_to_map_with_invalid_big_endian_length, fail_to_map_with_invalid_big_endian_total_struct_offset, create_and_delete_complex_map_once, + get_map_at_max_messages_returns_null, + remove_at_max_messages_is_safe, RECEIVE_TESTS);