Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Curl should make sure mbedtls having threading support #15500

Closed
wxiaoguang opened this issue Nov 6, 2024 · 2 comments
Closed

Curl should make sure mbedtls having threading support #15500

wxiaoguang opened this issue Nov 6, 2024 · 2 comments
Assignees
Labels

Comments

@wxiaoguang
Copy link

wxiaoguang commented Nov 6, 2024

When building libcurl with mbedtls, by default the mbedtls library doesn't enable its MBEDTLS_THREADING_C.

However, according to https://mbed-tls.readthedocs.io/en/latest/kb/development/thread-safety-and-multi-threading/#thread-safety , mbedtls PSA is not thread-safe.

And it seems that THREADING_SUPPORT in vtls/mbedtls.c doesn't really help, the concurrency crash still occurs in mbedtls library when using libcurl+mbedtls in a multiple-thread program without MBEDTLS_THREADING_C

So maybe it's good to require defined(MBEDTLS_THREADING_C) in curl's vtls/mbedtls.c when there is threading support?

Or at least, psa_crypto_init should be protected by the mutex? Actually I am not sure whether it is enough, I think requiring defined(MBEDTLS_THREADING_C) is the best approach.

Some related crash stacks:

0   libsystem_kernel.dylib        	0x00000001d24c02ec __pthread_kill + 8 (:-1)
1   libsystem_pthread.dylib       	0x00000001e62b3c0c pthread_kill + 268 (pthread.c:1721)
2   libsystem_c.dylib             	0x00000001917bfba0 abort + 180 (abort.c:118)
3   libsystem_malloc.dylib        	0x0000000199a03588 malloc_vreport + 896 (malloc_printf.c:251)
4   libsystem_malloc.dylib        	0x0000000199a031f8 malloc_report + 64 (malloc_printf.c:290)
5   libsystem_malloc.dylib        	0x0000000199a027b0 find_zone_and_free + 528 (malloc.c:2793)
6   xxxxxxxxxxxxxxx              	0x000000010287b200 mbedtls_md_free + 296
7   xxxxxxxxxxxxxxx              	0x0000000102875504 mbedtls_entropy_func + 520
8   xxxxxxxxxxxxxxx              	0x00000001028616c0 mbedtls_ctr_drbg_reseed_internal + 248
9   xxxxxxxxxxxxxxx              	0x000000010286197c mbedtls_ctr_drbg_seed + 272
10  xxxxxxxxxxxxxxx              	0x000000010289424c mbedtls_psa_drbg_seed + 56
11  xxxxxxxxxxxxxxx              	0x00000001028941d8 mbedtls_psa_random_seed + 60
12  xxxxxxxxxxxxxxx              	0x000000010288f4b8 mbedtls_psa_crypto_init_subsystem + 300
13  xxxxxxxxxxxxxxx              	0x000000010288f2bc psa_crypto_init + 120
14  xxxxxxxxxxxxxxx              	0x00000001027dc484 mbed_connect_step1 + 392
15  xxxxxxxxxxxxxxx              	0x00000001027dbfd8 mbed_connect_common + 224
16  xxxxxxxxxxxxxxx              	0x00000001027db814 mbedtls_connect_nonblocking + 48
17  xxxxxxxxxxxxxxx              	0x00000001027e47dc ssl_connect_nonblocking + 80
18  xxxxxxxxxxxxxxx              	0x00000001027e2dd0 ssl_cf_connect + 696
19  xxxxxxxxxxxxxxx              	0x0000000102767920 Curl_conn_cf_connect + 92
20  xxxxxxxxxxxxxxx              	0x000000010276e874 cf_setup_connect + 188
21  xxxxxxxxxxxxxxx              	0x0000000102767920 Curl_conn_cf_connect + 92
22  xxxxxxxxxxxxxxx              	0x00000001027612dc cf_hc_baller_connect + 84
23  xxxxxxxxxxxxxxx              	0x000000010275ffcc cf_hc_connect + 1084
24  xxxxxxxxxxxxxxx              	0x0000000102767bf0 Curl_conn_connect + 312
25  xxxxxxxxxxxxxxx              	0x00000001027a8888 multi_runsingle + 1820
26  xxxxxxxxxxxxxxx              	0x00000001027a7f34 curl_multi_perform + 280
27  xxxxxxxxxxxxxxx              	0x000000010278201c easy_transfer + 148
28  xxxxxxxxxxxxxxx              	0x0000000102781e64 easy_perform + 496
29  xxxxxxxxxxxxxxx              	0x0000000102781c68 curl_easy_perform + 32


0   xxxxxxxxxxxxxxx              	0x000000010489ff64 mbedtls_sha512_update + 64
1   xxxxxxxxxxxxxxx              	0x000000010486bbd0 mbedtls_md_update + 332
2   xxxxxxxxxxxxxxx              	0x0000000104865078 entropy_update + 320
3   xxxxxxxxxxxxxxx              	0x0000000104865224 entropy_gather_internal + 308
4   xxxxxxxxxxxxxxx              	0x0000000104865384 mbedtls_entropy_func + 136
5   xxxxxxxxxxxxxxx              	0x00000001048516c0 mbedtls_ctr_drbg_reseed_internal + 248
6   xxxxxxxxxxxxxxx              	0x000000010485197c mbedtls_ctr_drbg_seed + 272
7   xxxxxxxxxxxxxxx              	0x000000010488424c mbedtls_psa_drbg_seed + 56
8   xxxxxxxxxxxxxxx              	0x00000001048841d8 mbedtls_psa_random_seed + 60
9   xxxxxxxxxxxxxxx              	0x000000010487f4b8 mbedtls_psa_crypto_init_subsystem + 300
10  xxxxxxxxxxxxxxx              	0x000000010487f2bc psa_crypto_init + 120
11  xxxxxxxxxxxxxxx              	0x00000001047cc484 mbed_connect_step1 + 392
12  xxxxxxxxxxxxxxx              	0x00000001047cbfd8 mbed_connect_common + 224
13  xxxxxxxxxxxxxxx              	0x00000001047cb814 mbedtls_connect_nonblocking + 48
14  xxxxxxxxxxxxxxx              	0x00000001047d47dc ssl_connect_nonblocking + 80
15  xxxxxxxxxxxxxxx              	0x00000001047d2dd0 ssl_cf_connect + 696
16  xxxxxxxxxxxxxxx              	0x0000000104757920 Curl_conn_cf_connect + 92
17  xxxxxxxxxxxxxxx              	0x000000010475e874 cf_setup_connect + 188
18  xxxxxxxxxxxxxxx              	0x0000000104757920 Curl_conn_cf_connect + 92
19  xxxxxxxxxxxxxxx              	0x00000001047512dc cf_hc_baller_connect + 84
20  xxxxxxxxxxxxxxx              	0x000000010474ffcc cf_hc_connect + 1084
21  xxxxxxxxxxxxxxx              	0x0000000104757bf0 Curl_conn_connect + 312
22  xxxxxxxxxxxxxxx              	0x0000000104798888 multi_runsingle + 1820
23  xxxxxxxxxxxxxxx              	0x0000000104797f34 curl_multi_perform + 280
24  xxxxxxxxxxxxxxx              	0x000000010477201c easy_transfer + 148
25  xxxxxxxxxxxxxxx              	0x0000000104771e64 easy_perform + 496
26  xxxxxxxxxxxxxxx              	0x0000000104771c68 curl_easy_perform + 32
@icing
Copy link
Contributor

icing commented Nov 7, 2024

It seems a good idea to add the check when building curl. I propose #15505 for checking this.

Update: the check during compile time led to failures in CI jobs. Not all platforms have built mbedtls that way. Instead, the initialisation of psa_crypto_init() is now done on curl's global init and protected by mbedtls locks if available.

@icing icing self-assigned this Nov 7, 2024
@bagder bagder closed this as completed in bcf8a84 Nov 7, 2024
@wxiaoguang
Copy link
Author

Thank you for the quick response!

Although I was not sure whether protecting psa_crypto_init is enough, I read the code again and I think it should be also fine, the psa_crypto_init seems to be the only "psa" function call and all other mbedtls calls are all with "context/locker" which should be thread-safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants