(Suggested fix) pthread_local_storage_thread_deleted_callback is not conforming to the specification of pthread_key_create (IDFGH-4842)

[ivmarkov]

Environment

• All

Problem Description

• We are building support for the Rust standard library on top of ESP-IDF.
• During the implementation of the support for Rust threads, we were hit by a crash which - in our opinion - is caused by the implementation of pthread_local_storage_thread_deleted_callback
• The implementation is not really conforming to the specification as outlined here
• The concrete sentence in the specification which is not currently implemented correctly is, to quite the above link: "the value of the key is set to NULL, and then the function pointed to is called with the previously associated value as its sole argument" (emphasis mine)

Expected Behavior

pthread_local_storage_thread_deleted_callback should first delete the key from the linked list and then call the destructor with the original value of the key. Thus, if the destructor recursively calls into the pthread library (which is allowed by the spec), it will see a consistent local storage

Actual Behavior

pthread_local_storage_thread_deleted_callback calls the destructor without removing the entry from the list. In our code, it results in memory corruption issue, as the Rust-provided destructor calls-back into the pthread code and sees inconsistent local storage.

Steps to reproduce

Not really yet simple steps to reproduce, besides running our Rust-on-ESP32 demo code.

See the "Rough Steps" section where this issue is described, as well as the fix from below.

Fix:

Here

[ivmarkov]

Anybody? This is a bit of a blocker for enabling Rust STD support for ESP32 / ESP-IDF as demoed here.

[igrr]

@ivmarkov Sorry for not getting back to you sooner. Could you please submit your fix as a PR? It looks okay to me, if we don't find any test failures in CI we will merge it.

P.S. the demo looks really cool!

[ivmarkov]

Thanks a lot - I'll prepare a PR for that.

(BTW - I just updated the demo with type-safe Rust wrappers for WiFi, Httpd and Ping so I hope the coolness factor has grown a bit. J)

[projectgus]

Hi @ivmarkov,

I'm sorry, I picked this task up recently but somehow didn't see the lines with "Fix: Here" in the issue report (I literally thought "I wonder what they mean by 'Suggested fix', entirely my mistake for skim-reading). As a result, I've written a slightly different fix!

This fix is a little more complex as I've tried to also address the other missing behaviour for pthread_setspecific(): if a destructor function calls pthread_setspecific() to set a different key to a non-NULL value then the destructor should also be called for this key, but only after destructor functions have been called for the existing non-NULL values.

Patch is attached, if you have any comments or concerns before I submit it for merging then please let me know.

0001-pthread-Fix-behaviour-when-pthread-destructor-calls-.patch.txt

[ivmarkov]

@projectgus - I don't mind at all having your fix applied instead. I've quickly skimmed through it and it does seem to contain a fix similar to mine - which is - remove the entry from the list before calling its destructor.

I also applied your patch and then re-built my little Rust playground with it - seems to work fine in that spawning and joining Rust threads does not crash the app.

Many thanks for your work and taking my little request seriously!

===

Out of pure curiosity - why are you guys maintaining your own pthread-over-freertos support and not using this one?

[projectgus]

Hi @ivmarkov,

Good to hear, thanks for testing and confirming! This issue will be updated when it merges to master.

Out of pure curiosity - why are you guys maintaining your own pthread-over-freertos support and not using this one?

That's a very good question, I think one historical reason and one technical reason:

• Historical: At the time this pthread support was implemented in ESP-IDF the "FreeRTOS Plus" features were licensed differently. Since 2018 (I think) this project is MIT licensed, so no longer a valid reason.
• Technical: "Vanilla" FreeRTOS doesn't currently support SMP. However, this situation is currently changing as well. It's very conceivable that at a future time when FreeRTOS+POSIX supports the FreeRTOS SMP kernel, and ESP-IDF uses the FreeRTOS SMP kernel, we could switch over the implementation in ESP-IDF. I can't give any estimate of when this might be, though.

EDIT: Also it looks like pthread_setspecific/pthread_getspecific support is missing there, so we'd probably need to port our implementation over. Still not a bad idea, though.