FT_MOTION Fix possible nullptr dereference

[bmourit]

Description

While looking through the ft_motion code, looking to fix remaining bugs before the next release, I came across some areas that (I believe) could lead to undefined behavior. It is possible someone with a better understanding of this code can correct me if this is wrong, but from what I can see, there are some cases that exist that could lead to null pointer dereferencing and divide-by-zero cases. I may have added more checks than needed, especially if we know for certain these cases can never happen??

I'm hoping someone will double check this for me.

From what I can see, in the loop() function stepper.current_block can be set to nullptr by discard_planner_block_protected() when a block is finished processing (!blockProcRdy). However, the convertToSteps() function, which might still be processing the last batch of that same block (batchRdyForInterp is true), uses stepper.current_block->extruder to get the extruder number for axis_steps_per_mm. If stepper.current_block is nullptr at this point, it will cause a null pointer dereference, leading to a crash or undefined behavior, which could manifest as violent movement or a complete halt.

The other issue is if FTMotion::loadBlockData() receives a planner block (current_block) where current_block->millimeters (totalLength) is zero or extremely small, calculations like oneOverLength = 1.0f / totalLength or spm = totalLength / current_block->step_event_count can result in Inf or NaN.

If this is somehow impossible due to some other code I somehow missed, then we can close this. If not - here are some fixes.

Requirements

FT_MOTION should be enabled

Benefits

Prevents divide by zero (possible NaN) and handles very small or zero length blocks that would otherwise lead to undefined behavior.

Configurations

N/A

Related Issues

Might fix (needs testing):

• [BUG] FT_MOTION Stepper pauses an instant with intervals of about 1.2-1.3 seconds #27405
• [BUG] FT_MOTION causes violent Y movement #27535
• [BUG] FT_MOTION produces strange sound #27344

UPDATE!
For now, limiting this to the more likely case of possible nullptr dereference. The fix is fairly simple, using a cached extruder index in the problem area.

[narno2202]

Glad to see that someone is looking at FT_MOTION.
As I understand the current code, for me, stepper.current_block is never nullptr in converToSteps (in VSCode, GPT 4.1 gives me the same answer).
Regarding the other issue, the floating number represents millimeters, not sure that some blocks could be nanometers.

[bmourit]

Glad to see that someone is looking at FT_MOTION. As I understand the current code, for me, stepper.current_block is never nullptr in converToSteps (in VSCode, GPT 4.1 gives me the same answer). Regarding the other issue, the floating number represents millimeters, not sure that some blocks could be nanometers.

Hi! Thanks for the response. Could you explain to me how a nullptr is not possible? I still don't see it (Sorry, maybe I'm slow).

[narno2202]

You're totally right, I look again at the code. In theory, a nullptr dereference could only occur after a runout block. I never have such issue but it seems possible, so this PR could prevent firmware crash. To avoid a new variable, we can use active_extruder instead of stepper.current_block->extruder.

[bmourit]

Sounds great to me.

You're totally right, I look again at the code. In theory, a nullptr dereference could only occur after a runout block. I never have such issue but it seems possible, so this PR could prevent firmware crash. To avoid a new variable, we can use active_extruder instead of stepper.current_block->extruder.

I've invited you to have access, since you are the expert on the FTM code.

[thinkyhead]

In general we should always be looking at current_block before calling something that might reference it, or within the called method to make sure it still has a value. But, if some value from the block is needed beyond the block's lifetime, we must cache it someplace.

So, the extra cached value is fine. It makes sense to keep values from the current_block around for as long as the current block still matters, where it is being referenced by some stepper routine that needs it, or if where is something that might be called when there is no current block but which wants to reference the previously-fetched block.

With FT Motion, the current planner block only needs to live long enough to get converted to the DIR/STEP queue, so we should add comments in the FTM code to indicate places where the current_block could potentially be zeroed out or advanced to the next block in the midst of that conversion process. That will clarify why this patch is needed, and why something similar might be needed again in future.

[vovodroid]

Could it be related to [BUG] FT_MOTION produces strange sound #27344?