Potential thread stack overflow in AP_SmartAudio::loop

[mirusu400]

Bug report

Issue details

While some detailed build configuration, There are potential stack overflow in thread function named AP_SmartAudio::loop.

ardupilot/libraries/AP_VideoTX/AP_SmartAudio.cpp

Lines 52 to 70 in b36f539

	_port = AP::serialmanager().find_serial(AP_SerialManager::SerialProtocol_SmartAudio, 0);
	if (_port!=nullptr) {
	_port->configure_parity(0);
	_port->set_stop_bits(AP::vtx().has_option(AP_VideoTX::VideoOptions::VTX_SA_ONE_STOP_BIT) ? 1 : 2);
	_port->set_flow_control(AP_HAL::UARTDriver::FLOW_CONTROL_DISABLE);
	_port->set_options((_port->get_options() & ~AP_HAL::UARTDriver::OPTION_RXINV)
	| AP_HAL::UARTDriver::OPTION_HDPLEX | AP_HAL::UARTDriver::OPTION_PULLDOWN_TX | AP_HAL::UARTDriver::OPTION_PULLDOWN_RX);
	if (!hal.scheduler->thread_create(FUNCTOR_BIND_MEMBER(&AP_SmartAudio::loop, void),
	"SmartAudio",
	768, AP_HAL::Scheduler::PRIORITY_IO, -1)) {
	return false;
	}
	AP::vtx().set_provider_enabled(AP_VideoTX::VTXType::SmartAudio);
	return true;
	}
	return false;
	}

In this line, AP_SmartAudio::loop Allows 768 stack size.

However, after manually checking stack, it might have 1040 Bytes in with some detailed configurations.

Steps to produce

1. In boards.py, Add this line:

# In 224 line..
        env.CFLAGS += [
++          '-fstack-usage',
            '-ffunction-sections',
            '-fdata-sections',
            '-fsigned-char',
...
# In 343 line...
        env.CXXFLAGS += [
++          '-fstack-usage',
            '-std=gnu++11',

            '-fdata-sections',

This will help you automatically calculating stack size of each function.

2. Build ardupilot normally, using this configurations:

./waf configure --board AnyleafH7 -g --check-verbose --disable-Werror --toolchain=arm-none-eabi --notests --enable-scripting --enable-opendroneid --enable-check-firmware --enable-custom-controller --enable-gps-logging --enable-header-checks --enable-stats 
./waf -v rover

Now we can get stack usage file (*.su) for each source file, So we can manually check stack size of each function.

In case of AP_SmartAudio::loop:

There are large call stack with this flow:

AP_SmartAudio::loop (_ZN13AP_SmartAudio4loopEv) => 64 size
AP_VideoTX::announce_vtx_settings (_ZNK10AP_VideoTX21announce_vtx_settingsEv) => 32 size
GCS::send_text (_ZN3GCS9send_textE12MAV_SEVERITYPKcz) => 0 size
GCS::send_textv (_ZN3GCS10send_textvE12MAV_SEVERITYPKcSt9__va_listh) => 160 size
GCS::service_statustext (_ZN3GCS18service_statustextEv) => 24 size
GCS_MAVLINK::service_statustext (_ZN11GCS_MAVLINK18service_statustextEv) => 120 size
_mav_finalize_message_chan_send (_mav_finalize_message_chan_send) => 88 size
mavlink_sign_packet (_Z19mavlink_sign_packetP17__mavlink_signingPhPKhhS3_hS3_) => 136 size
mavlink_sha256_update (_Z21mavlink_sha256_updateP18mavlink_sha256_ctxPKvm) => 416 size

SUM => 1040 bytes

So, there are potentially occur stack overflow in AP_SmartAudio::loop Thread function.

Version

Commit 0e72fc7

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

I've only checked Rover yet, but I think all platforms have same issues.

Hardware type
AnyleafH7

[tridge]

nice find, thanks!
we could either allocate the sha256 context for the mavlink signing separately, or you could just increase the stack here:
https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_VideoTX/AP_SmartAudio.cpp#L59

can you open a PR for that?

[mirusu400]

@tridge Sure! is just changing stack size enough?

[peterbarker]

On Tue, 15 Oct 2024, mirusu400 wrote: @tridge Sure! is just changing stack size is enough?

Problem with just increasing stack size is that we have a *lot* of threads, and adding 512 bytes to each of them is a significant hit to resources on smt32.

[timtuxworth]

nice find, thanks! we could either allocate the sha256 context for the mavlink signing separately

mavlink_sha256_update uses 416 bytes on the stack! mavlink_sha256_ctx only seems to be 90 bytes and "zero" seems to be 72 bytes. Where is the rest?

[peterbarker]

On Fri, 18 Oct 2024, Tim Tuxworth wrote: nice find, thanks! we could either allocate the sha256 context for the mavlink signing separately mavlink_sha256_update uses 416 bytes on the stack! mavlink_sha256_ctx only seems to be 90 bytes and "zero" seems to be 72 bytes. Where is the rest?

You know there's only 768 bytes allowed, right? ... and the information *is* there :-)

[timtuxworth]

On Fri, 18 Oct 2024, Tim Tuxworth wrote: nice find, thanks! we could either allocate the sha256 context for the mavlink signing separately mavlink_sha256_update uses 416 bytes on the stack! mavlink_sha256_ctx only seems to be 90 bytes and "zero" seems to be 72 bytes. Where is the rest?
You know there's only 768 bytes allowed, right? ... and the information is there :-)

That's kind of my point. If it's correct then one single method is consuming 416 bytes out of 768 bytes of stack space. I'm saying this is what should be fixed, but I can't see how just allocating the context would do it. There is still 300ish bytes being put on the stack even once you take account of the context.