[BUG] Memory Access problem, solveable by additional sanity check

[bkubicek]

Did you test the latest bugfix-2.0.x code?

No, but I will test it now!

Bug Description

There exists no validation for the array size of "SWITCHING_TOOLHEAD_X_POS" in Configuration.h.
With more than two toolheads, this can lead to a memory access problem in tool_change.cpp, line 507 "grabxpos = toolheadposx[new_tool];"
It will query a "random" x position for a toolhead whose index is larger than the array size from configuration.h.
I am not super familiar with the way the sanity checks are done, so am sadly disabled to add a appropriate sanity check myself.
Additionally, Software endstops were enabled, and not regarded during toolhead changes. Which might or might not be on purpose, depending if the parking locations are supposed to be in "printable" area or not by definition.

Bug Timeline

No response

Expected behavior

Toolhead E3 does not crash into x boundaries, especially with software endstops enabled

Actual behavior

With software endstops enabled, changing the toolhead should not cause the toolhead to move to a "random" x position outside of software endstop limits.
During compiletime, it should be checked if there exists a defined parking position for each of the toolheads.

Steps to Reproduce

No response

Version of Marlin Firmware

2.0.9.3

Printer model

similar e3d toolchanger system, corexy, 3 toolheads

Electronics

BOARD_BTT_OCTOPUS_V1_1

Add-ons

No response

Bed Leveling

ABL Bilinear mesh

Your Slicer

No response

Host Software

No response

Additional information & file uploads

No response

[GMagician]

you may try with:
static_assert(COUNT(toolheadposx) == EXTRUDERS, "SWITCHING_TOOLHEAD_X_POS must be an array EXTRUDERS long.");
just after: constexpr float toolheadposx[] = SWITCHING_TOOLHEAD_X_POS;

[GMagician]

this kind of check is also done outside sanitycheck.h, for some other arrays

[GMagician]

I just saw now that SWITCHING_TOOLHEAD_X_POS array is also used in other parts of code. Maybe it's better to really add this somewhere to sanitycheck.h:

#if ANY(SWITCHING_TOOLHEAD, MAGNETIC_SWITCHING_TOOLHEAD, ELECTROMAGNETIC_SWITCHING_TOOLHEAD)
  constexpr float thpx[] = SWITCHING_TOOLHEAD_X_POS;
  static_assert(COUNT(thpx) == EXTRUDERS, "SWITCHING_TOOLHEAD_X_POS must be an array EXTRUDERS long.");
#endif

[thisiskeithb]

Closing since a PR was created: #23985

[bkubicek]

thanks a bunch!

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.