add integer bound check

[RamReso]

When sending mavlink command_long messages with some arbitrary parameters, I noticed random crashes in the simulation. I looked into it and found, that the simulation always crashes, when I send a parameter >215 or <-215 for a coordinate. This was due to a integer over/underflow in the conversion from long to int.

I added the necessary checks to the converting function and now I do not longer notice the problem.

[tpwrules]

It is strange, but not impossible, that this should crash. What platform are you running on and what message are you trying to send?

The check needs to be moved to only be done if stores_location and after the multiplication (but before the cast).

[RamReso]

I'm not 100% sure what you mean by platform. I'm only using a simulation which I start with:
python Tools/autotest/sim_vehicle.py -v ArduCopter -D

I encounter the problem if I send the command_long over mavlink using a self written python program with the following parameters:

target_system = 1
target_component = 1
command = 201 (MAV_CMD_DO_SET_ROI)
confirmation = 0
param_1= 841.8709017045903
param_2: -355.8708939554573
param_3: -487.38928814332417
param_4: -605.9047446662618
param_5: 409.52030363804556
param_6: -237.83399241328152
param_7: -691.756644619793 

These parameter values are random, but they should not lead to a crash.

I also noticed the same problems using different command, e.g. 195-MAV_CMD_DO_SET_ROI_LOCATION

The stack trace is the following:

convert_COMMAND_LONG_loc_param GCS_Common.cpp:5154
GCS_MAVLINK::convert_COMMAND_LONG_to_COMMAND_INT GCS_Common.cpp:5174
GCS_MAVLINK::try_command_long_as_command_int GCS_Common.cpp:5140
GCS_MAVLINK::handle_command_long GCS_Common.cpp:5195
GCS_MAVLINK::handle_message GCS_Common.cpp:4271
GCS_MAVLINK_Copter::handle_message GCS_Mavlink.cpp:1523
GCS_MAVLINK::packetReceived GCS_Common.cpp:1835
GCS_MAVLINK_Copter::packetReceived GCS_Mavlink.cpp:646
GCS_MAVLINK::update_receive GCS_Common.cpp:1887
GCS::update_receive GCS_Common.cpp:2707
Functor<void>::method_wrapper<GCS, &GCS::update_receive> functor.h:88
Functor::operator() functor.h:54
AP_Scheduler::run AP_Scheduler.cpp:264
AP_Scheduler::loop AP_Scheduler.cpp:393
AP_Vehicle::loop AP_Vehicle.cpp:544
HAL_SITL::run HAL_SITL_Class.cpp:289
main Copter.cpp:957

Thank you for your recommendation regarding the position of the check. I will adjust the code accordingly.

[tpwrules]

Thanks for providing the command you used. By platform I mean processor type, operating system name and version, compiler name and version, etc. Ardupilot SITL is usually run on Ubuntu x86_64 so that's the least likely to cause problems.

Thanks also for updating the code, but there are a lot of casts in Ardupilot so it's concerning that this one should crash. This is undefined behavior so we are clearly in the wrong, but I'd like to be able to replicate the problem myself and better determine impact on common platforms and what other changes we might need to make.

[tpwrules]

Well I expected this to be a semi-exotic platform but sure enough it's easily triggerable on x86_64 (using my Nix flake). You can use mavproxy and the command module load message ; message COMMAND_LONG 1 1 201 0 841 -355 -487 -605 409 -237 -691 . Sure enough it takes a SIGFPE in convert_COMMAND_LONG_loc_param which causes the SITL to dump core and abort.

It also happens on a non-debug build (you selected a debug build with -D). It does not happen on ChibiOS (CubeOrange, non-debug build). It doesn't happen either on the linux target.

I will discuss this with the team and see if this is the right fix and if we need to do more.

• Commit message needs prefixing with the subsystem (https://ardupilot.org/dev/docs/submitting-patches-back-to-master.html#preparing-commits)
• Does INT32_MAX/MIN round correctly when converted to float to avoid leaving a gap?
• We should probably apply this for the non-location path too, though location values are much more likely to be out of range

[tridge]

I think @peterbarker should look at this, though I think the fix is correct. I would have used constrain_float() though.

[tridge]

Note that we deliberately ignore SIGFPE on non-SITL. If you want to see what a real flight controller would do in a situation like this then set "SIM_FLOAT_EXCEPT = 0" and SITL will ignore FPE in the same way a real flight controller does.
We deliberately deviate from the behaviour of a real flight controller by default in SITL to help us find cases like this.

[IamPete1]

Much larger change, but we should probably reject the command in this case.

[RamReso]

• Commit message needs prefixing with the subsystem (https://ardupilot.org/dev/docs/submitting-patches-back-to-master.html#preparing-commits)

Done.

[RamReso]

Much larger change, but we should probably reject the command in this case.

I agree with that. But I'm not sure if I can implement this by my own.

[peterbarker]

Done properly this would be a bool function with an int32& return parameter.

OTOH, I didn't do that when I created the function for its intended purpose, cope with NaNs in the input data.

I'm well-aware of this particular issue. I would note that we do get reports of this happening from time-to-time - but that means that people are actually passing in bad values into SITL from their external control systems. If we merge this then we will no longer provide the rather brutal feedback that we currently do, which is to segfault. That increases the chances that someone's external control software will pass through SITL testing without catching bugs, which is bad.

Better to modify the call chain to be able to return a MAV_RESULT so we can detect these issues and pass a failure message back to the GCS.

[peterbarker]

This isn't great behaviour; we don't want vehicles deciding to fly to 0,0 because someone's screwed up on the GCS side and we interpret their bad data as 0,0. We should be rejecting the command.

[peterbarker]

Suggested change

	float convertedValue = param *1e7;
	const float convertedValue = param *1e7;

[peterbarker]

I've created #28866 to replace this one. I have added @RamReso as a Co-authored-by on the relevant patch.

My PR doesn't squash the value to zero when invalid, instead returning a useful return code to the GCS. It also more-tightly validates the latitude/longitudes being passed in - instead of ensure it fits into an int32_t it instead checks they're valid lat/lng, which is a strictly stronger check.

@RamReso could you check my PR to see if you like it, please?