Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validation to MSP for the rc_smoothing_auto_factor setting #9560

Conversation

etracer65
Copy link
Member

Fixes #9559

This is a workaround for a validation bug in configurator 10.6 where the user can enter an invalid value, immediately click "Save" and that value will be stored before the value is range-checked. Added validation to the MSP message processing to constrain the range to prevent a dangerous result.

@@ -3051,7 +3051,7 @@ static mspResult_e mspProcessInCommand(mspDescriptor_t srcDesc, int16_t cmdMSP,
if (sbufBytesRemaining(src) >= 1) {
// Added in MSP API 1.42
#if defined(USE_RC_SMOOTHING_FILTER)
configRebootUpdateCheckU8(&rxConfigMutable()->rc_smoothing_auto_factor, sbufReadU8(src));
configRebootUpdateCheckU8(&rxConfigMutable()->rc_smoothing_auto_factor, constrain(sbufReadU8(src), RC_SMOOTHING_AUTO_FACTOR_MIN, RC_SMOOTHING_AUTO_FACTOR_MAX));
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a comment in here stating that this is a workaround for a configurator bug, and probably a TODO to pull this out again once a fixed configurator has been released?
I don't think starting the proliferation of input validation being done in the MSP receiver is a good move.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can add a comment about why it's there. But I don't think we can safely remove this any time soon. Are we planning on releasing a patch release for 10.6 configurator? Even if we do people we can't guarantee that everybody will upgrade. And in this case the problem is bad enough not to risk.

Copy link
Member

Choose a reason for hiding this comment

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

I think we will have to consider a patch release of configurator - we cannot guarantee that users will upgrade from a vulnerable version of 4.1 any more than we can guarantee that they will upgrade configurator, so a two-pronged approach will give us best coverage. At least in the case of configurator they will get an indication when starting up that a new version is available.

This is a workaround for a validation bug in configurator 10.6 where the user can enter an invalid value, immediately click "Save" and that value will be stored before the value is range-checked. Added validation to the MSP message processing to constrain the range to prevent a dangerous result.
@etracer65 etracer65 force-pushed the add_msp_rc_smoothing_auto_factor_validation branch from 426f4dc to 15ec707 Compare March 7, 2020 23:49
@mikeller mikeller merged commit 55a0c01 into betaflight:master Mar 9, 2020
mikeller added a commit that referenced this pull request Mar 9, 2020
…tor_validation

Add validation to MSP for the rc_smoothing_auto_factor setting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants