Does USB device code for STM32 really allocate dynamic memory from interrupts?

[freddie_chopin]

Posted on January 13, 2017 at 18:29

Today in one of my projects I've enabled a new feature of my RTOS (

http://distortos.org/

) which ensures some functions are not used in interrupts. This checks - among others - for any use of mutexes from interrupts, as it is a logical error to do that (first of all - interrupts cannot ''block'' and additionally using mutexes in interrupts violates the concept of ''ownership'').

You may imagine how surprised I was when the critical error was raised even before the application would fully start... See this call stack:

It turns out that ST's code for USB device (CDC, audio, HID, DFU, MSC) in STM32 happily allocates dynamic memory in interrupts... The path is like this:

• OTG_FS_IRQHandler() interrupt,

• HAL_PCD_IRQHandler(),

• HAL_PCD_SetupStageCallback(),

• USBD_LL_SetupStage(),

• USBD_StdDevReq(),

• USBD_SetConfig(),

• USBD_SetClassConfig(),

• indirect call of a function stored in ''Init'' field of USBD_ClassTypeDef struct.

The ''Init'' pointer in 

USBD_ClassTypeDef struct is initialized with an address of functions like 

USBD_CDC_Init(), and these function - almost right at the beginning - call malloc() via USBD_malloc() macro...

As malloc() is generally _NOT_ reentrant, it has to be protected from concurrent access from multiple contexts/threads. If a real-time operating system is used, this protection is usually implemented with a mutex or pausing the scheduler (preventing context switches). In single threaded applications malloc() is usually not protected at all, because most of the people are reasonable enough and don't even consider using malloc() in interrupts. In both of these cases adding ST's USB device code sets the application on a path to certain failure if it actually uses dynamic allocation anywhere else:

• when locking is done with mutexes in a multithreaded system - any attempt to block in interrupt handler (like locking a locked mutex used to serialize access to malloc()) is undefined behaviour and will most likely severely corrupt the system, 

  worse - as mutexes serializing access to malloc() are usually recursive, such call can lead to a typical race condition, causing heap corruption,

• when locking is done by pausing the scheduler in a multithreaded system - interrupts are not affected by that locking mechanism, malloc() may be re-entered from two different contexts, leading to heap corruption,

• in a single-threaded application with no locking - an allocation in interrupt during an allocation in main thread will lead to heap corruption.

This whole discussion obviously also applies to free(), which is called in interrupt context via at least 3 different code paths...

#malloc #memory-corruption #race-condition #heap #rtos #usb

[Ernesto B]

Posted on February 05, 2017 at 17:45

Hi there, any thoughts about this question?

[valentin]

Posted on February 07, 2017 at 20:21

Wow, I had no idea about malloc on interrupts. I hadn't used it in them yet - based on a gut feeling. I guess that feeling was right

This makes me worry about using the ST HAL usb code though. Because the init function is called once the user plugs in the usb cable, this means that in theory, malloc can be called at any time from within an ISR. And I am using malloc/free in some of my threads (mutex protected).

If anybody comes up with a solution, please let me know / post it here. I need to make sure that my finished product is stable at all times!

Maybe I can protect it using taskEnterCritical / ExitCritical instead? Doesn't that macro temporarily disable interrupts as well? (FreeRTOS).

Edit: yes, according to FreeRTOS docs, taskEnter/ExitCritical() macros would be a workaround here. They disable (mask) all interrupts below priority 5 (per default) and thus also disable any other context switching.

Unfortunately, that doesn't help with distortos, I presume? Maybe you can implement a similar critical section macro that disables all context switches AND interrupts temporarily?

[freddie_chopin]

Posted on February 19, 2017 at 09:58

Hello

Roehrig.Valentin

Valentin wrote:

If anybody comes up with a solution, please let me know / post it here. I need to make sure that my finished product is stable at all times!

Maybe I can protect it using taskEnterCritical / ExitCritical instead? Doesn't that macro temporarily disable interrupts as well? (FreeRTOS).

Edit: yes, according to FreeRTOS docs, taskEnter/ExitCritical() macros would be a workaround here. They disable (mask) all interrupts below priority 5 (per default) and thus also disable any other context switching.

You can do that, but this is more like a 'work around' than a solution. In my project I've just replaced malloc with static allocation - I know how many device classes will be active at any time, so this is not an issue. The problem here is that such approach should be the default one, not the other way around...

Valentin wrote:

Unfortunately, that doesn't help with distortos, I presume? Maybe you can implement a similar critical section macro that disables all context switches AND interrupts temporarily?

There are objects for creating critical sections - distortos::InterruptMaskingLock - which uses RAII pattern. And you could use that to protect malloc, but - as I've said above - this is just a work around of the original problem. There are things you just don't do, and allocating memory from the interrupt handler is one of them. Doing that kinda ruins the main concept of the RTOS - predictable behaviour with low latency. Blocking all interrupts to allocate memory is obviously not going to lower the latency or the jitter - quite the opposite...

[Krasi Gichev]

Posted on May 03, 2017 at 08:25

I agree - original code is not good. Allocation in real-time (or more precisely high-availability) systems must be avoided not only in interrupts, but totally not used after the initialization phase. This must be followed quite strictly by 'libraries', 'frameworks', 'stacks' and all kind of shared components (reusable ones) - because in other way those components will 'pollute' the final application. 

There are ways to avoid using heap at all, at least in the 'component' level and keep it out in the main (integration/application/project) level - if needed. 

I might be wrong but locking interrupts may solve the problem only if we use it globally for all heap operations - if 'task' level code is running and starts an allocation, then it is preempted by usb interrupt inside the malloc, we come to the same conflict. Another way to solve this is to use multiple heaps for conflicting parts - e.g. separate task heap and interrupt heap. And going this way we can find the real solution - to correctly implement static allocation outside of the interrupts.

[valentin]

Posted on May 16, 2017 at 05:44

******, now I'm also stuck at that problem:

Using FreeRtos, it now fires the OTG_HS_IRQHandler() interrupt after calling MX_USB_DEVICE_Init();

That results in a failed assert because I overloaded malloc() to make it thread safe:

void* malloc(size_t size) {
 return pvPortMalloc(size);
}�?�?�?

See here:

Does anyone know a workaround? Can I preallocate the memory for the usb struct in USBD_CDC_Init()?

pdev->pClassData = USBD_malloc(sizeof (USBD_CDC_HandleTypeDef));�?

[anonymous.8]

Posted on June 05, 2017 at 23:10

Take a look at UM1734 - STM32Cube USB Device Library User Manual.

At the end it discusses how to use static memory allocation in your device class init functions.

[valentin]

Posted on June 05, 2017 at 23:32

Thanks for the hint, I will take a look at that.

I've had it changed to static allocation now anyway.

I replaced this in usbd_cdc.c:

// pdev->pClassData = USBD_malloc(sizeof (USBD_CDC_HandleTypeDef)); //fix: alocate statically
 pdev->pClassData = &ClassData;�?�?

And added this in usbd_cdc.h:

/* fix: allocate memory statically */
USBD_CDC_HandleTypeDef ClassData;�?�?

Problem solved.

@ST: Would it be possible to maybe introduce a define somewhere e.g. in mxconstants.h about dynamic vs static allocation here?

I see that dynamic allocation is intriguing so that the memory is only used after the usb cable is actually plugged in.

But together with FreeRTOS is would be better to have it default to static allocation IMHO.

[Lix Paulian]

Posted on January 19, 2018 at 23:42

This is a solution with limited value. How would your solution deal with a case when you use both USB peripherals as devices? Or if you have more than one device class?

[Ant M]

Posted on October 27, 2017 at 17:10

I agree that It really should be static by default, or maybe put user code areas in the relevant file?

[Andrei Chichak]

Posted on January 20, 2018 at 19:55

I spent a few weeks tracking this down as well.

I am integrating a USB CDC device into a system that uses RTEMS as the RTOS. 

Once I tracked down the cause, malloc in an interrupt context, I checked with some of my friends that do embedded systems for medical devices, fitness trackers, consumer goods, and the sort, and the general consensus was 'THEY DID WHAT? NO don't ever use malloc in an interrupt routine. What were they thinking?'

To be sure that it wasn't just the unanimous opinion of 6 random embedded programming professionals, I went to the authors of RTEMS to understand what there problem was. They write:

It is definitely returning NULL because

you shouldn't malloc from an ISR. malloc() is a non-deterministic operation. I am

surprised that code was designed to do that.

My workaround came down to a few lines of code that avoided using malloc and free. I noticed that the malloc just allocated enough memory for a single 

USBD_HandleTypeDef, the code used one instance of it, and then freed it when the USB disconnected. This could EASILY be accomplished by using a single global variable in my space:

USBD_HandleTypeDef

usbHandle;

Then, in usb_cdc.c, instead of doing the malloc:

pdev->

pClassData

= (

void

*) &usbHandle;

and instead of doing the free:

pdev->

pClassData

= NULL;

This will always work.

There are a few more damning issues with the original design. Even though malloc is called in an interrupt routine, the null result gives a return code of -1 to the caller of USBD_CDC_INIT, but it does not get checked or acted upon. The null pointer just gets used, even though the lower level routines tried to tell the callers that there was an error.

Finally, the various functions are set up to return an uint8_t but actually return a USBD_StatusTypeDef enum - sometimes. Enums are not uint8_ts, they are enums. If you want to return an enum, return an enum.

Once again, this code sucked up a lot of unbillable time. 

Andrei

[Pavel A.]

Posted on January 21, 2018 at 16:01

In the specific case of ST device-side examples this allocation does not actually use malloc in Cube generated project (as in the CubeMX repository v, 1.9.0). Instead, it creates 'USBD_static_malloc' which just returns a static buffer. And it never can fail.

For one instance it is OK

But as general design issue, all this should be moved out of the interrupt handler to a 'task' (or whatever it's called in your OS).

- pa

[jschloss]

This just broke my code too, after I turned on the thread safety locks in newlib (__malloc_lock) for freertos. This is very broken, malloc isn't threadsafe or reentrant on newlib by default! I hacked a static array for CDC driver like the earlier posters, and it seems to be working now, but this is a big deal. The code is not safe as-is.

Is there a better place to report bugs in the HAL drivers? Newlib's heap does not support being used like this.

[anonymous.8]

On 2019-08-12 3:46 p.m., ST Community wrote:

[AToth]

MISRA C: avoid malloc at all cost.

USB configuration is known in advance so everything can be allocated during compilation.

Again, poor/bloated SW from ST.

[Lix Paulian]

MISRA-C, they're living in the past. The point here is not necessarily that they use malloc() and free(), but that they do it on an interrupt context!

[Ernesto B]

You haven't been involved in any critical device, have you? MISRA avoids mallocs because it introduces ways of failure in the application, undefined and non deterministic behaviours and, of course, leaves the door open to memory leaks. Those problems are definitely not in the past.

[Lix Paulian]

You bet I was involved. Not really those where lives might be in danger, but still critical. What MISRA ignores is that the times where you had 32K RAM or so on a controller are long gone. A microcontroller today might have more RAM as an IBM PC of the eighties. Dynamic allocation used carefully might even be beneficial to a system as memory can be re-used. Would you imagine the PC-software of today built with no dynamic allocation? Embedded systems are going the same route as the PC, only 10 to 15 years delayed.

I am not advocating the blind use of dynamic allocation, but I can't stand those paranoics that are forbidding it. There are cases where, if judiciously used, dynamic allocation can be an asset and not a liability.