Return int from platform mutex_init #439

2025-09-04T15:52:31-05:00

gilles-peskine-arm commented

2025-09-04 15:52:31 -05:00

(Migrated from github.com)

Change the platform mutex_init function to return an error code. This allows us to keep track of whether a mutex has been initialized, and thus to react safely if mutex initialization failed. This isn't an issue in pthread, but it is one with many other threading implementations. With this change, we don't have to hope that the platform functions are doing this for us.

If I was designing the interface from scratch, I'd make the API function mbedtls_mutex_init also return the error code. But I'm not sure there's a way to do that in the limited amount of time that we have, even if we keep ignoring the return value in our own code for now.

Thus, as far as I'm concerned, this resolves https://github.com/Mbed-TLS/mbedtls/issues/8455.

This is part of a chain, due to how metatest.c is stored in the framework, impacts crypto, and is built in mbedtls.

PR checklist

changelog provided
framework PR provided https://github.com/Mbed-TLS/mbedtls-framework/pull/205
mbedtls development PR not required because: crypto (platform) only
mbedtls 3.6 PR provided (documentation improvements only)
tests provided

Change the platform `mutex_init` function to return an error code. This allows us to keep track of whether a mutex has been initialized, and thus to react safely if mutex initialization failed. This isn't an issue in pthread, but it is one with many other threading implementations. With this change, we don't have to hope that the platform functions are doing this for us. If I was designing the interface from scratch, I'd make the API function `mbedtls_mutex_init` also return the error code. But I'm not sure there's a way to do that in the limited amount of time that we have, even if we keep ignoring the return value in our own code for now. Thus, as far as I'm concerned, this resolves https://github.com/Mbed-TLS/mbedtls/issues/8455. This is part of a chain, due to how `metatest.c` is stored in the framework, impacts crypto, and is built in mbedtls. 1. https://github.com/Mbed-TLS/mbedtls/pull/10388 2. https://github.com/Mbed-TLS/mbedtls-framework/pull/205 3. https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/439 (main objective) 4. https://github.com/Mbed-TLS/mbedtls/pull/10389 ## PR checklist - [x] **changelog** provided - [x] **framework PR** provided https://github.com/Mbed-TLS/mbedtls-framework/pull/205 - [x] **mbedtls development PR** not required because: crypto (platform) only - [x] **mbedtls 3.6 PR** provided (documentation improvements only) - **tests** provided

gilles-peskine-arm commented

2025-09-05 03:39:25 -05:00

(Migrated from github.com)

Note that this isn't ready for review yet. I'm still trying to figure out what makes it not as simple as was thinking yesterday.

mpg (Migrated from github.com) reviewed

2025-09-08 03:39:16 -05:00

mpg (Migrated from github.com) left a comment

Looks pretty good to me. A few minor nits, but also some thoughts on the design: we have no idea what the value of initialized is before mutex_init() has been called. I think that's OK for the limited goal we have here, but we should be aware of the limitations.

Looks pretty good to me. A few minor nits, but also some thoughts on the design: we have no idea what the value of `initialized` is before `mutex_init()` has been called. I think that's OK for the limited goal we have here, but we should be aware of the limitations.

ChangeLog.d/threading.txt

Partially pre-existing, I was on the fence about it when reviewing the previous PR already: we might want to split this into "Requirement changes" (condition variables) and "API changes" (the rest).

drivers/builtin/src/threading.c

					
				@ -158,21 +153,32 @@ int (*mbedtls_mutex_unlock_ptr)(mbedtls_platform_mutex_t *) = threading_mutex_fa

That seems completely unsafe: assume I create a variable of type mbedtls_threading_mutext_t on the stack. Then the initial value of its initialized member will be whatever was on in this stack location previously, and in particular it might be 1. Then in

mbedtls_threading_mutex_t m; // on the stack; m.initialized just happens to be 1
mbedtls_mutex_init(&m):

actual initialization of the underlying platform mutex would be skipped.

Actually this seems like a larger concern with the initialized field: we have no idea what its value is before we call init. In particular it might happen to be 1 by accident.

Well (thinking while writing) this is only an issue before mutext_init has been called. After that, the field has a well-defined value. And I don't think we mean to catch the kind of misuse where people forget to cal init - we only mean to cover the case where people do call mutex_init(), but internally platform_init() fails and this can't be reported back to the caller.

So I think it's fine as long as we don't try reading the initialized field before mutex_init() has been called - in particular we shouldn't do things like this line, which fortunately is removed in a later commit.

That seems completely unsafe: assume I create a variable of type `mbedtls_threading_mutext_t` on the stack. Then the initial value of its `initialized` member will be whatever was on in this stack location previously, and in particular it might be 1. Then in ```c mbedtls_threading_mutex_t m; // on the stack; m.initialized just happens to be 1 mbedtls_mutex_init(&m): ``` actual initialization of the underlying platform mutex would be skipped. Actually this seems like a larger concern with the `initialized` field: we have no idea what its value is before we call `init`. In particular it might happen to be 1 by accident. Well (thinking while writing) this is only an issue before `mutext_init` has been called. After that, the field has a well-defined value. And I don't think we mean to catch the kind of misuse where people forget to cal init - we only mean to cover the case where people do call `mutex_init()`, but internally `platform_init()` fails and this can't be reported back to the caller. So I think it's fine as long as we don't try reading the `initialized` field before `mutex_init()` has been called - in particular we shouldn't do things like this line, which fortunately is removed in a later commit.

include/mbedtls/threading.h

					
				@ -66,4 +61,4 @@

				 * \note            The library will always unlock a mutex from the same

				 *                  thread that locked it, and will never lock a mutex

				 *                  in a thread that has already locked it.

				 *

Just checking: it's implicit that the behaviour is undefined if the function is called concurrently from multiple threads, because that's covered by the fact that double-init is undefined, right?

Minor: "and has not been destroyed" (like for mutex_init)?

Minor: "and has not been destroyed" (like for `mutex_init`)?

Typo: the the

Minor: I'm not sure that "if A or B are not both" is entirely correct syntax, but I think it's clear enough in practice.

include/mbedtls/threading.h

					
				@ -81,0 +94,4 @@

				 *                      if a thread is waiting on it, or if this function

				 *                      is called concurrently from multiple threads.

				 * \param cond_signal   The condition variable signal implementation. <br>

				 *                      The behavior is undefined if the condition variable

Question: why do we mention "if called concurrently from multiple threads" here but not for init? Here too "called from multiple threads" seems to be covered by "already destroyed" I think?

Question: why do we mention "if called concurrently from multiple threads" here but not for `init`? Here too "called from multiple threads" seems to be covered by "already destroyed" I think?

tests/suites/test_suite_platform_threading.function

					
				@ -31,6 +31,35 @@ exit:

				}

Note: this is a double free, but we're not in the test function about double free. Should we switch to a single free here for the sake of orthogonality?

gilles-peskine-arm (Migrated from github.com) reviewed

2025-09-08 04:57:18 -05:00

ChangeLog.d/threading.txt

I wouldn't consider platform interfaces to be an API (A stands for “application”, after all), but that does seem to be our historical practice, so I'll split it.

gilles-peskine-arm (Migrated from github.com) reviewed

2025-09-08 05:04:16 -05:00

include/mbedtls/threading.h

					
				@ -81,0 +94,4 @@

				 *                      if a thread is waiting on it, or if this function

				 *                      is called concurrently from multiple threads.

				 * \param cond_signal   The condition variable signal implementation. <br>

				 *                      The behavior is undefined if the condition variable

I was just inconsistent. I'll err on the side of explicitness.

gilles-peskine-arm (Migrated from github.com) reviewed

2025-09-08 05:07:10 -05:00

drivers/builtin/src/threading.c

					
				@ -158,21 +153,32 @@ int (*mbedtls_mutex_unlock_ptr)(mbedtls_platform_mutex_t *) = threading_mutex_fa

Oh, right, this was fundamentally flawed. The init function just can't read anything from the object.

mpg (Migrated from github.com) reviewed

2025-09-08 05:10:59 -05:00

ChangeLog.d/threading.txt

(A stands for “application”, after all)

Right. I think we're using "API changes" with the more general meaning of "you'll need to change your own code when upgrading", whether "you" is a normal application writer or an integrator. I'm not sure if there's a well recognised term for that.

> (A stands for “application”, after all) Right. I think we're using "API changes" with the more general meaning of "you'll need to change your own code when upgrading", whether "you" is a normal application writer or an integrator. I'm not sure if there's a well recognised term for that.

mpg (Migrated from github.com) reviewed

2025-09-08 05:42:11 -05:00

mpg (Migrated from github.com) left a comment

No description provided.

LGTM, thanks!

gilles-peskine-arm commented

2025-09-08 09:28:41 -05:00

(Migrated from github.com)

I rebased to get the merge of the framework PR. I also fixed the typo Manuel found. The previous version is in https://github.com/gilles-peskine-arm/TF-PSA-Crypto/tree/threading-1.0-mutex-init-crypto-3

I rebased to get the merge of the framework PR. I also fixed the [typo Manuel found](https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/439#discussion_r2329851004). The previous version is in https://github.com/gilles-peskine-arm/TF-PSA-Crypto/tree/threading-1.0-mutex-init-crypto-3

mpg (Migrated from github.com) approved these changes

2025-09-09 02:56:30 -05:00

mpg (Migrated from github.com) left a comment

LGTM, thanks!

valeriosetti (Migrated from github.com) approved these changes

2025-09-09 06:29:00 -05:00

Sign in to join this conversation.

No reviewers

mpg

valeriosetti

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Mbed-TLS/TF-PSA-Crypto!439

				`@ -158,21 +153,32 @@ int (mbedtls_mutex_unlock_ptr)(mbedtls_platform_mutex_t ) = threading_mutex_fa`

				`@ -31,6 +31,35 @@ exit:`
				`}`

Rows
Columns