Concern on the usage of malloc() on USB Host MSC driver library

[BParh.1]

I have concern on the usage of malloc() in the generated USB Host MSC library.

In file usbh_msc.c, take a look at the usage of USBH_malloc)_:

static USBH_StatusTypeDef USBH_MSC_InterfaceInit(USBH_HandleTypeDef *phost)
{
     .............
    MSC_HandleTypeDef *MSC_Handle;
    .........
    phost->pActiveClass->pData = (MSC_HandleTypeDef *)USBH_malloc(sizeof(MSC_HandleTypeDef));
    MSC_Handle = (MSC_HandleTypeDef *) phost->pActiveClass->pData;
 
   if (MSC_Handle == NULL)
   {
     USBH_DbgLog("Cannot allocate memory for MSC Handle");
     return USBH_FAIL;
   }
}

where it is defined as following on usbh_conf.h:

/** Alias for memory allocation. */
#define USBH_malloc         malloc

My questions:

1. Why it uses dynamic location malloc()? To my understanding for real time application this is risky.
2. Why currently the malloc() return NULL memory allocation? I have defined my heap size 0x2000 in the CubeMX project config which should suffice to serve the requsted 256 byte (i.e. sizeof(MSC_HandleTypeDef)).
3. Why in the code, I never see this memory freed? It would lead to potential memory leak right?

[Pavel A.]

> Why it uses dynamic location malloc()?

It does not call malloc. It calls USBH_malloc. Users are expected to implement it it as they want.

> It would lead to potential memory leak right?

Right.

This USB "middleware" is not of product quality, it only demonstrates working of the hardware. The risk is acceptable for this use.

If you are after a quality driver for a real product, look somewhere else.

-- pa

[BParh.1]

Well, in that generated code as I show the snippet, the USBH_malloc() lead to malloc. This is generated code, whatever user change will be overridden. I disagree to claim this generated driver is not meant for quality, I don't think ST sees that way, and this community shoud be part of community effort to improve

[waclawek.jan]

> This is generated code, whatever user change will be overridden.

Then make it your own code.

In most cases, static allocation is appropriate there, as you will have only one instance of the given struct.

> I disagree

Read the licence to find out ST's stance on the matter.

> this community shoud be part of community effort to improve

Yes, you are free to contribute.

JW

[BParh.1]

What was the original concern to address such that malloc() is used here which I might miss? That would help the us to make better approach.

This might be a stupid question, but what about allocate it as local variable?

>Read the licence to find out ST's stance on the matter.

I understand It is eventually our own risk and responsibility for the code we use. ST cannot be hold legally for this, However it is somewhere in ST website to state that it is committed to help user to prototype the product faster to focus on the business application logic, not the driver.

From the kind of response so far, I feel the need to state that I have no intention to put blame on anyone, ST has been doing great jobs with this community and the tool to help developers. The focus here is for people to be aware of the risk and discuss together better approach. Some people might think malloc() risk is acceptable, but probably not to others especially in real time application, so it is good at least to understand the risk and discuss potential better approach.

[Pavel A.]

> what about allocate it as local variable?

Yes, some ST examples use local variables instead of malloc.

https://github.com/STMicroelectronics/STM32CubeF0/blob/4390ff6bfb693104cf97192f98c3dc9e3a7c296a/Projects/STM32F072B-Discovery/Applications/USB_Device/HID_Standalone/Src/usbd_conf.c#L453

[BParh.1]

Thank you @Pavel A.​ , also anyway I find other alternative that is to use the FreeRTOS version to allocate memory pvPortMalloc() and vPortFree() to free it.

I think this should be a safer alternative than malloc().