Infinite loop in AP_HAL/utility/ftoa_engine when formatting floats with very small exponents

[JasonMDavey]

Bug report

Issue details
Invoking ftoa_engine (low-level function responsible for formatting floating point numbers, used by GCS::send_text) enters an infinite loop when invoked as below:

char *buf = (char*)malloc(255);
ftoa_engine(1e-44f, buf, 7, 7);

This appears to happen with other very small numbers (around e-44 exponent), and results in an infinite loop here, as prod and decimal are both zero:

ardupilot/libraries/AP_HAL/utility/ftoa_engine.cpp

Line 149 in ae48665

while ((prod -= decimal) >= 0)

Adding the following to the start of the function seems to be a workable fix, although it may break the formatting of very small (denormalized) floats:

if (fabsf(val) < FLT_MIN) val = 0.0f;

Version
Plane Stable 4.6.2

Platform
[x] All
[ ] AntennaTracker
[ ] Copter
[ ] Plane
[ ] Rover
[ ] Submarine

Airframe type
n/a

Hardware type
n/a

Logs
n/a

[tpwrules]

Fun discovery, thanks. I can replicate in Lua too (though then it only freezes the Lua thread).

Your solution seems good as an initial fix. I don't see a situation where denormals not being formatted would make or break a flight. But longer term we could switch to a better formatting engine. There are relatively recent developments in the field. (I was looking into this recently cause I also want a better string -> float routine.)

I think we do also keep denormals enabled on hardware. I don't know if that has a performance penalty on Cortex cores, but if it has a severe one it may be safer to turn on flush-to-zero; it's supported by the Cortex FPUs. But I would have to do some serious thought on how it might mangle e.g. the EKF state or screw up comparisons.

[peterbarker]

@JasonMDavey I was unable to replicate the problem through the caller to ftoa_engine. I'm guessing you saw this when calling send_text - could you lend a hand in fixing up the new regression test (test_vsnprintf) actually replicate?

.... @tpwrules what call did you make in Lua to replicate?

[tpwrules]

> ("%.99f"):format(1e-43)
0.00000000000000000000000000000000000000000004974610
> ("%.99f"):format(1e-44)
< no response and freeze >

Using my Lua REPL applet on Cube Orange but there shouldn't be any difference using a regular Lua script. Denormals can get into the platform-specific weeds sometimes so this may not work on SITL, I have not tried.

[peterbarker]

> ("%.99f"):format(1e-43)
0.00000000000000000000000000000000000000000004974610
> ("%.99f"):format(1e-44)
< no response and freeze >

Using my Lua REPL applet on Cube Orange but there shouldn't be any difference using a regular Lua script. Denormals can get into the platform-specific weeds sometimes so this may not work on SITL, I have not tried.

Thanks! That's replicated now.

[JasonMDavey]

@JasonMDavey I was unable to replicate the problem through the caller to ftoa_engine. I'm guessing you saw this when calling send_text - could you lend a hand in fixing up the new regression test (test_vsnprintf) actually replicate?

Were you able to get this working? For me, your test_vsnprintf code does replicate the hanging:

(gdb) run
Starting program: /ardupilot/build/sitl/tests/test_vsnprintf 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[==========] Running 2 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 2 tests from vsnprintf_Test
[ RUN      ] vsnprintf_Test.Basic
[       OK ] vsnprintf_Test.Basic (0 ms)
[ RUN      ] vsnprintf_Test.LargeNegativeExponent
^C
Program received signal SIGINT, Interrupt.
0x000055555562216e in ftoa_engine (val=9.80908925e-45, buf=0x7fffffffde20 "", precision=7 '\a', maxDecimals=60 '<') at ../../libraries/AP_HAL/utility/ftoa_engine.cpp:152
152                 while ((prod -= decimal) >= 0)
(gdb) bt
#0  0x000055555562216e in ftoa_engine (val=9.80908925e-45, buf=0x7fffffffde20 "", precision=7 '\a', maxDecimals=60 '<') at ../../libraries/AP_HAL/utility/ftoa_engine.cpp:152
#1  0x0000555555574bba in print_vprintf (s=0x7fffffffde90, fmt=0x55555568e181 "", ap=0x7fffffffdf00) at ../../libraries/AP_HAL/utility/print_vprintf.cpp:208
#2  0x0000555555574573 in AP_HAL::Util::vsnprintf (this=0x555555704360 <utilInstance>, str=0x7fffffffe020 "\260\340\377\377\020", size=256, format=0x55555568e17c "%.99f", ap=0x7fffffffdf00)    
    at ../../libraries/AP_HAL/Util.cpp:60
#3  0x00005555555744de in AP_HAL::Util::snprintf (this=0x555555704360 <utilInstance>, str=0x7fffffffe020 "\260\340\377\377\020", size=256, format=0x55555568e17c "%.99f")
    at ../../libraries/AP_HAL/Util.cpp:50
#4  0x0000555555572933 in vsnprintf_Test_LargeNegativeExponent_Test::TestBody (this=0x555555785c20) at ../../libraries/AP_HAL/tests/test_vsnprintf.cpp:69
...

And thanks for the quick turnaround on the fix!

[peterbarker]

@JasonMDavey I was unable to replicate the problem through the caller to ftoa_engine. I'm guessing you saw this when calling send_text - could you lend a hand in fixing up the new regression test (test_vsnprintf) actually replicate?

Were you able to get this working? For me, your test_vsnprintf code does replicate the hanging:

Yes - but my branch should also stop it hanging! Were you testing my entire branch or just a subset of the patches? I can't make the line number in ftoa_engine.c match up to either master or my branch.... so I'm hoping you had local changes :-)

And thanks for the quick turnaround on the fix!

Well, this is a very serious bug! @tpwrules has done a much more in-depth investigation and doesn't think we have code in master which would cause a vehicle to fall out of the sky (it requires the user to specify a lot of decimal places), but if the bug triggers on the main thread your flying vehicle won't be flying for long.