[BUG] xEventGroupSetBits returns garbage if another high priority thread freed container

[skotopes]

Describe the bug

https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/event_groups.c#L643

pxEventBits->uxEventBits used after xTaskResumeAll(); which will return garbage if another thread freed underlying container.

Target

• Development board: any
• Instruction Set Architecture: any
• IDE and version: any
• Toolchain and version: any

Host

• Host OS: any
• Version: any

To Reproduce

• In high priority thread:
  • create event group
  • call xEventGroupWaitBits
  • delete event group
• In low priority thread:
  • call xEventGroupSetBits
  • check return value

Expected behavior
Not returning garbage

Screenshots
none

Additional context
none

[aggarg]

This is a case of use after free and seems like an application bug. The application needs to ensure that no other task is using a resource before deleting it.

[skotopes]

@aggarg I believe it's not: I'm using EventGroup to signal from the child thread to the parent thread that it exited. I'm not using this primitive anymore, but because xEventGroupSetBits dereferencing pxEventBits->uxEventBits I get garbage in return or mpu fault(we protect various memory regions).

In general value returned from xEventGroupSetBits is mess: depending on thread priorities and xEventGroupWaitBits flags you'll get completely random results. Copying pxEventBits->uxEventBits value before xTaskResumeAll will make more sense. Any reason not to do so?

[CookiePLMonster]

This is a case of use after free and seems like an application bug. The application needs to ensure that no other task is using a resource before deleting it.

Wouldn't the pattern mentioned by this bug report be reasonably common in apps? The pattern of

1. Create/Wake up Thread B from Thread A and start it
2. Wait for Thread B to finish work
3. Free resources from Thread A

isn't anything out of ordinary, and it's more or less exactly what this issue refers to.

[aggarg]

Copying pxEventBits->uxEventBits value before xTaskResumeAll will make more sense. Any reason not to do so?

We can do that. I'd still suggest to evaluate your design and see if you really need to free resources dynamically.

Would you like to raise a PR for this or would you like us to raise one?

[CookiePLMonster]

We can do that. I'd still suggest to evaluate your design and see if you really need to free resources dynamically.

The use case in question cannot use a globally allocated event group, in this case a local variable is used - so while it's not utilizing FreeRTOS' dynamic allocation, the fact the static buffer is placed on the stack still makes it somewhat dynamic.

[skotopes]

Copying pxEventBits->uxEventBits value before xTaskResumeAll will make more sense. Any reason not to do so?

We can do that. I'd still suggest to evaluate your design and see if you really need to free resources dynamically.

Would you like to raise a PR for this or would you like us to raise one?

If it's ok with you I can make PR.

[aggarg]

If it's ok with you I can make PR.

Yes, please do.

[aggarg]

The use case in question cannot use a globally allocated event group, in this case a local variable is used - so while it's not utilizing FreeRTOS' dynamic allocation, the fact the static buffer is placed on the stack still makes it somewhat dynamic.

Thanks for explaining.