STMicroelectronics / STM32CubeF4

STM32Cube MCU Full Package for the STM32F4 series - (HAL + LL Drivers, CMSIS Core, CMSIS Device, MW libraries plus a set of Projects running on all boards provided by ST (Nucleo, Evaluation and Discovery Kits))
Other
871 stars 418 forks source link

UART race condition bug associated with HAL_LOCK() function location #164

Closed taehyun9203 closed 11 months ago

taehyun9203 commented 1 year ago

Caution The Issues are strictly limited for the reporting of problem encountered with the software provided in this project. For any other problem related to the STM32 product, the performance, the hardware characteristics and boards, the tools the environment in general, please post a topic in the ST Community/STM32 MCUs forum.

Describe the set-up

Describe the bug A clear and concise description of what the bug is.

Hello, I'd like to address an issue regarding the placement of the lock function (__HAL_LOCK(huart)) for the UART device. While conducting tests on the Nucleo-F429ZI board, I discovered that the current location of the lock function could potentially result in a race condition when a user employs two different UART receive functions. The reason behind this potential race condition is that the UART receiver function locks the UART after checking the receiver state only once, without conducting additional checks when locking the UART.

To mitigate this race condition, I propose relocating the lock function before checking the UART receiver state. Specifically, I suggest moving line 1382 of code in HAL_UART_Receive_DMA to line 1372, and moving line 1239 of code in HAL_UART_Receive to line 1229 in HAL_UART_Receive.

I'd like to note that although I previously posted an article in the STM community, I unintentionally omitted a discussion about the lock function. To provide a more comprehensive explanation of the race condition stemming from the current location of the lock function, I have attached a picture instead of sharing the actual code.

In summary, while a bug triggered by an experienced programmer may not occur frequently, the current placement of the lock function deviates from the original intention and could potentially result in erroneous behavior. Therefore, I recommend fixing it to ensure safer code execution.

How To Reproduce

  1. Indicate the global behavior of your application project.

  2. The modules that you suspect to be the cause of the problem (Driver, BSP, MW ...).

  3. The use case that generates the problem.

  4. How we can reproduce the problem.

To begin, I configured UART3 with the DMA option on the board. The code snippet below illustrates my test approach.

unnamed

My assumption was that by calling the UART_Receive_DMA function in the code, the data reception would exclusively employ the DMA method.

This belief was rooted in the fact that the invocation of UART_Start_Receive_DMA() through HAL_UART_Receive_DMA() establishes the receiver state (RxState) as HAL_UART_STATE_BUSY_RX, as indicated in line 3259 of the second image. unnamed (1)

Consequently, if I were to subsequently call HAL_UART_Receive()—a non-DMA receiving method—it wouldn't be able to retrieve any data if HAL_UART_Receive_DMA() had been invoked earlier. This limitation arises because HAL_UART_Receive() can only capture data when the receiver state is idle (RxState == HAL_UART_STATE_READY), as evident in line 1271 of the code excerpt.

unnamed (2)

My thought process led me to the idea that after calling HAL_UART_Receive_DMA(), manually setting the RxState to HAL_UART_STATE_READY could potentially enable HAL_UART_Receive() to function correctly.

unnamed (2)

I tested this theory, but the results were unexpected. When I transmitted the input string "Hello world!!" via serial communication to the UART, the UART failed to receive the first character of the string. unnamed

I suspect that the issue stems from HAL_UART_Receive_DMA() and HAL_UART_Receive() both locking the UART after checking the Receiver state (line 1374 of HAL_UART_Receive_DMA and line 1231 of HAL_UART_Receive).

image unnamed (3)

Since neither receiving method verifies the RxState following the lock, a race condition can occur. To ensure the safe utilization of UART, it might be prudent to place the __HAL_LOCK function ahead of the RxState check. I'm eager to hear your thoughts on this matter.

Additional context If you have a first analysis or patch correction, thank you to share your proposal.

Screenshots If applicable, add screenshots to help explain your problem.

ASELSTM commented 1 year ago

Hi @taehyun9203,

Thank you fir this report and for this detailed analysis. However, would you please make sur you are using the latest version of the STM32CubeF4 available on GitHub.

Referring to the screenshot you have posted, the code snippet of the HAL_UART_Receive_DMA() function dates back to the version v1.25.2. In fact, a work has been done on the driver to remove unnecessary __HAL_LOCK()/__HAL_UNLOCK() in order to avoid such a race condition in the frame of this commit.

So would you please double check the version of the STM32CubeF4 you are using.

With regards,

taehyun9203 commented 11 months ago

Thank you for answering my question.

I checked the code is removed in current version.

I tested that code old STM32CUBEIDE version.

I closed this question.