Why does the HAL USB CDC driver call malloc in an ISR context?

[jschloss]

Since __malloc_lock has not been implemented by default, newlib's malloc / _malloc_r is not reentrant or thread safe (including ISR safe). This seems like a bug.

This will cause very bad interactions with user code use of malloc, including heap corruption. Additionally, most RTOS's heaps are not ISR-safe, so when using the freertos heap it causes a crash. Usually when using newlib & freertos I implement _mallor_r to use the rots heap, and provide __malloc_lock and __malloc_unlock so it is threadsafe. This is necessary so the C library internal allocations are threadsafe, but doing so causes a crash when the cdc driver attempts to allocate memory within the ISR.

Can this be changed to allow either a statically allocated buffer, a user provided buffer, or a thread-context dynamically allocated buffer? I have moved the classData it is trying to allocate to be static for now, but in general this needs to be allocated per usb instance, so a single one is not enough.

Perhaps a user provided buffer passed in at configuration time would be better.

[S.Ma]

static uint8_t USBD_CDC_Init (USBD_HandleTypeDef *pdev, uint8_t cfgidx)

...

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

IMOH avoiding using dynamic memory allocation is preffered (lower risks).

The USBB_CDC_Init is used as function pointer. This is where usually I create a double pointer which I call a "hook": The first pointer is the function, the second is the context, the pointer to the structure containing all what is needed by the function to operate, and is passed as parameter (as most HAL functions do)

[Pavel A.]

USBD_malloc is not the malloc. It is a different function. Implement it as your like.

-- pa

[jschloss]

Yes, my point is under FreeRTOS/newlib it is not really possible to provide a general ISR safe malloc, at least not without making a lot of compromises. I think this is similar for most platforms. I can't use either the C stdlib's malloc nor my RTOS's malloc.

The examples do default USBD_malloc to newlib's malloc, which is not a good idea. It likely works in the examples because there are no other allocations happening, but as soon as another context starts calling malloc (or printf, which will call malloc on first use, etc) there is risk of heap corruption. It seems like this needs to be more robust.

I could write my own heap implementation that is ISR safe, but that seems a bit unexpected to be necessary, especially for a single fixed size allocation. It would be very easy to pre-allocate this in a normal execution context.

[jschloss]

That is interesting - I am using the STM32H7 libraries (STM32Cube_FW_H7_V1.3.0), and in usbd_conf_template.h there is

#define USBD_malloc malloc

However, when I open STM32Cube_FW_L4_V1.13.0 and v1.11.0 's usbd_conf_template.h I also see

#define USBD_malloc malloc

Which version of the L4 middleware do you have? Maybe this was changed at some point? I'll download a few more and see if I can find one with USBD_static_malloc.

I do see USBD_static_malloc in the nucleo example projects in my L4 folders, but not in the library itself, or in the code that cube generated for me - I didn't start this project from an example, and cube didn't copy one over for me when I generated the startup code.

doing a ~/STM32Cube/Repository $ grep -r USBD_static_malloc only returns hits in the L4 examples too, none in the H7 example projects. Actually, looking at this more, I don't see any pre-generated USB examples in the H7 cube projects folder.

Maybe this is the source of my confusion - The default in the middleware folder is malloc (so I thought that would be ok), and I never looked at the L4 examples since I am using an H7 part, and everyone just "knows" that they need to use the USBD_static_malloc from the nucleo examples, haha.

[jschloss]

So basically what @S.Ma​  said, if the library doesn't actually want a full malloc implementation, it would be a lot less confusing if it wasn't called malloc, and didn't default to malloc in the templates. It was a bit worrying to me initially to provide a malloc that returned the same static array pointer, since without inspecting the library I wasn't sure it was really only called once.

I'll switch to something equivalent to the USBD_static_malloc in the L4 example code. Thanks for the help everyone!