FreeRTOS / FreeRTOS-Kernel

FreeRTOS kernel files only, submoduled into https://github.com/FreeRTOS/FreeRTOS and various other repos.
https://www.FreeRTOS.org
MIT License
2.54k stars 1.07k forks source link

[BUG] Stack could be unaligned if portSTACK_GROWTH > 0 #753

Closed ivq closed 11 months ago

ivq commented 11 months ago

The stack of a task could not be properly aligned if stack grows upwards(portSTACK_GROWTH > 0).

Possible call trace of the bug:

  1. xTaskCreateStatic() creates a task, given stack buffer is only aligned as StackType_t.
  2. prvInitialiseNewTask() initializes the task, including the stack. The current implementation does not align the stack for portSTACK_GROWTH > 0. If portBYTE_ALIGNMENT is stricter than that of StackType_t, this will lead to unaligned stack or assert error if configASSERT is enabled.

Target None. The bug is only theoretically possible.

Host Not applicable.

To Reproduce Not applicable.

Expected behavior The stack should be aligned as portSTACK_GROWTH < 0 does.

Proposed Fix https://github.com/FreeRTOS/FreeRTOS-Kernel/pull/751

aggarg commented 11 months ago

If portBYTE_ALIGNMENT is stricter than that of StackType_t

StackType_t is always defined according to the portBYTE_ALIGNMENT. The following are the 4 ports for which portSTACK_GROWTH > 0:

  1. portable\ThirdParty\Community-Supported-Ports\CCS\C2000_C28x
    #define portSTACK_TYPE  uint16_t
    #define portBYTE_ALIGNMENT      2
  2. portable\SDCC\Cygnal
    #define portSTACK_TYPE  uint8_t
    #define portBYTE_ALIGNMENT          1
  3. portable\MPLAB\PIC24_dsPIC
    #define portSTACK_TYPE  uint16_t
    #define portBYTE_ALIGNMENT          2
  4. portable\MPLAB\PIC18F\portmacro.h
    #define portSTACK_TYPE  uint8_t
    #define portBYTE_ALIGNMENT          1

Therefore, no special alignment related adjustment is needed. Even if a new port defines the StackType_t incorrectly, the assert will catch it.

ivq commented 11 months ago

Yes, for all current ports for which portSTACK_GROWTH > 0, that's true. But I don't see any requirement on the binding of portBYTE_ALIGNMENT and StackType_t. Many ports for which portSTACK_GROWTH < 0 don't satisfy the requirement. For example:

  1. portable/IAR/ARM_CM4F/portmacro.h
    #define portBYTE_ALIGNMENT    8
    #define portSTACK_TYPE    uint32_t
  2. portable/GCC/RISC-V/portmacro.h
    // RV64, non E extension
    #define portBYTE_ALIGNMENT      16
    #define portSTACK_TYPE          uint64_t

The patch is indeed for future new ports.

aggarg commented 11 months ago

The patch is indeed for future new ports.

Okay, that is reasonable.