Skip to content

Improve error handling in scan_offsets function by replacing panic#1160

Merged
peterbarker merged 2 commits into
ArduPilot:masterfrom
Benkia:benkia/fix-panic-dfindexer
Mar 16, 2026
Merged

Improve error handling in scan_offsets function by replacing panic#1160
peterbarker merged 2 commits into
ArduPilot:masterfrom
Benkia:benkia/fix-panic-dfindexer

Conversation

@Benkia

@Benkia Benkia commented Jan 10, 2026

Copy link
Copy Markdown
Contributor

Hi

Its my first contribution here :)

I have an app that run on multiple bin files and I've noticed that when one of the files is corrupted the app exits with

bad header 0x2211 at 18054
bad header 0x1118 at 18055
bad header 0x186c at 18056
bad header 0x6ced at 18057
bad header 0xed50 at 18058
bad header 0x50a3 at 18059
Invalid length in FMT message at 18060

What I've noticed it that the C code dfinex.c is exitsing when it has issue with the header or format
But when I am using "PYMAVLINK_FAST_INDEX=0" and using the python code the behavior is changed and it only print message -

DFFormat: Unsupported format char: '�' in message �M\xa4�

So I've changed the code to print only
Let me know if thats OK or you would want to see something else

@Benkia

Benkia commented Jan 29, 2026

Copy link
Copy Markdown
Contributor Author

Hey @peterbarker @robertlong13
Kind reminder
Is there anything that I can add to help you understand it?

@robertlong13

Copy link
Copy Markdown
Contributor

Is there anything that I can add to help you understand it?

No you're good, I've just dropped the ball on reviewing this fully. I'll try to get to it in the next couple days

@robertlong13 robertlong13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your right that a panic is going too far. It should be a break so that you can actually still use the log up to the first point of corruption. But it needs to be a break, not a continue.

Comment thread dfindexer/dfindexer.c Outdated
Comment on lines +109 to +110
i++;
continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your right that a panic is going too far. It should be a break so that you can actually still use the log up to the first point of corruption. But it needs to be a break, not a continue.

Suggested change
i++;
continue;
break;

The python code breaks for these failure types, and for good reason. We cannot continue and hope to resync and salvage the rest. There's no fixed marker and no CRC, so the entire chain breaks down if we keep trying after a failure like this. If a single byte gets corrupted, and we'll end up scanning for the next byte that happens to be a valid message id (which is a large percentage of bytes), then we'll jump by the length of that supposed message (i.e., jump by a random value) and end up inside the body of another random message until we maybe get lucky and hit the real head of a message.

Now, the python side doesn't actually have this exact same check (the FMT length check), but the same logic applies, for this and the unknown msg type error. This case is impossible to hit without a byte corruption and we can't keep indexing past this point.

Comment thread dfindexer/dfindexer.c Outdated
Comment on lines +120 to +121
i++;
continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same suggestion as above

Suggested change
i++;
continue;
break;

Benkia added 2 commits March 11, 2026 15:13
Stop scanning at the first point of corruption rather than
continuing byte-by-byte, which could produce false positive
offsets from message payload bytes matching header patterns.
@Benkia Benkia force-pushed the benkia/fix-panic-dfindexer branch from 9fe8d1c to 336004e Compare March 11, 2026 13:13
@Benkia Benkia requested a review from robertlong13 March 11, 2026 13:13
@Benkia

Benkia commented Mar 11, 2026

Copy link
Copy Markdown
Contributor Author

@robertlong13 Done

@robertlong13 robertlong13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. @peterbarker may want you to squash the commits, but the code is good.

@peterbarker peterbarker merged commit 93489bc into ArduPilot:master Mar 16, 2026
23 checks passed
@peterbarker

Copy link
Copy Markdown
Contributor

Merged, thanks!

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.

3 participants