Pa_Initialize(): Guard against recursive calls. #794

daschuer · 2023-03-14T22:36:40Z

This fixes a stack overflow when a diver uses portaudio as well like FlexAsio

Fixes: #766

dechamps · 2023-03-14T23:21:12Z

The proposed code returns paNoError on a recursive call. I don't think that's wise - this situation should never be allowed to occur and it's important to bail out of "undefined behaviour" territory as soon as possible instead of attempting to continue. It would be best to return an error if recursion is detected. (In the case of the FlexASIO-related issue, this will make FlexASIO fail to initialize, which is what we want - FlexASIO should not be allowed to continue with a PortAudio library that is not the one that it came bundled with, as that can cause undefined behaviour and lead to more problems.)

A new error code could be added to the PaErrorCode enum for this case, or if that seems excessive I guess paInternalError could be returned instead.

daschuer · 2023-03-15T00:14:59Z

Done

src/common/pa_front.c

daschuer · 2023-03-15T23:15:24Z

Done.

RossBencina · 2023-03-20T22:30:01Z

Looking good. I think we should add a new error code. Perhaps call it paCanNotInitializeRecursively but maybe someone has a better idea for the name. That would be added here:

portaudio/include/portaudio.h

Line 155 in 65e5b82

} PaErrorCode;

With a new error text case added here:

portaudio/src/common/pa_front.c

Line 456 in ed922d9

default:

src/common/pa_front.c

This fixes a stack overflow when a diver uses portaudio as well like FlexAsio

RossBencina · 2023-03-28T00:48:48Z

src/common/pa_front.c

+    {
+        // a concurrent initialization is already running
+        PA_DEBUG(("Attempting to re-enter Pa_Initialize(), aborting!\n"));
+        result = paInternalError;


Please add new error code as described here: #794 (comment)

src/common/pa_front.c

RossBencina

I approve. I fixed a typo.

RossBencina · 2023-03-31T23:21:40Z

@dechamps @MichalPetryka I think this is ready to merge. Please could you confirm that it fixes #766 or advise if you have any objections. If you're happy use the GitHub review feature to approve it.

dechamps · 2023-04-02T01:31:55Z

The code looks like by me. I'll let @MichalPetryka confirm that it fixes #766 as they're the reporter.

If you're happy use the GitHub review feature to approve it.

I don't think I have the project permissions to do that. (It's either that or I can't find the button.)

MichalPetryka · 2023-04-02T07:23:12Z

src/common/pa_front.c

@@ -154,6 +154,7 @@ static PaUtilHostApiRepresentation **hostApis_ = 0;
 static int hostApisCount_ = 0;
 static int defaultHostApiIndex_ = 0;
 static int initializationCount_ = 0;
+static int initializing_ = 0;


Should this be marked volatile to prevent the compiler from doing threading unsafe optimizations here?

My original version had this volatile due to the assumption that this will prevent the compiler form reordering.
However this assumption is not guaranteed and fortunately also not necessary, because the compiler cannot move code across external function calls like InitializeHostApis() in our case.

Another aspect is thread safety. The volatile keyword can be part of the solution but, we need also atomic test_and_set() which is not available in C89. Since thread safety is not a requirement here and also not build in any other call the using code is responsible anyway and it is no point in this PR.

I think that initialization and shutdown are rare enough that using a mutex here to ensure thread safety would be okay. @RossBencina What do you say?

A mutex will create a deadlock in the recursive case.
IMHO this PR is good enough, protecting Portaudio in concurrent situation is not required for the original problem.

This is a good discussion of thread safety. But currently PortAudio is not thread safe. This fix prevents a crash caused by a stack overflow. It does not solve the thread safety issue but it is a step in the right direction. We can consider full thread safety in a later PR.

I think that initialization and shutdown are rare enough that using a mutex here to ensure thread safety would be okay.

This is a reasonable point. However, PortAudio is a single threaded API, and this patch is intended to prevent recursion on a single thread. According to our API semantics Pa_Initialize() may not be called concurrently on two threads, and even our initializationCount_ mechanism is not thread-safe. Furthermore, we currently have no portable mutex infrastructure so adding a mutex here is a non-trivial change.

So my vote would be to merge this as-is and worry about whether initialize/terminate, or indeed the whole API, should be made thread-safe as a separate issue.

philburk · 2023-04-03T22:47:04Z

src/common/pa_front.c

@@ -154,6 +154,7 @@ static PaUtilHostApiRepresentation **hostApis_ = 0;
 static int hostApisCount_ = 0;
 static int defaultHostApiIndex_ = 0;
 static int initializationCount_ = 0;
+static int initializing_ = 0;


This is a good discussion of thread safety. But currently PortAudio is not thread safe. This fix prevents a crash caused by a stack overflow. It does not solve the thread safety issue but it is a step in the right direction. We can consider full thread safety in a later PR.

RossBencina · 2023-04-03T22:49:03Z

@MichalPetryka outside your multi-threaded thread-safety concerns could you please confirm that this patch fixes the originally identified issue #766.

RossBencina · 2023-04-03T22:53:06Z

I don't think I have the project permissions to do that. (It's either that or I can't find the button.)

@dechamps Go to the "Files Changed" tab and at the top right there is a green "Review Changes" button.

RossBencina · 2023-04-04T00:56:42Z

I've created #808 for discussing multi-thread issues.

MichalPetryka · 2023-04-04T02:36:14Z

@MichalPetryka outside your multi-threaded thread-safety concerns could you please confirm that this patch fixes the originally identified issue #766.

Yeah, it does.

daschuer force-pushed the recursive_initalize branch 2 times, most recently from b34f98c to 2dabaa6 Compare March 15, 2023 00:14

daschuer force-pushed the recursive_initalize branch 3 times, most recently from 86a241e to 92625ce Compare March 15, 2023 09:24

dechamps reviewed Mar 15, 2023

View reviewed changes

src/common/pa_front.c Show resolved Hide resolved

src/common/pa_front.c Show resolved Hide resolved

daschuer force-pushed the recursive_initalize branch from 307ae20 to 9f718fd Compare March 15, 2023 14:02

MichalPetryka reviewed Mar 15, 2023

View reviewed changes

src/common/pa_front.c Outdated Show resolved Hide resolved

RossBencina changed the title ~~Pa_Initalze(): Guard against recursive calls.~~ Pa_Initialze(): Guard against recursive calls. Mar 20, 2023

RossBencina added the src-common Common sources in /src/common label Mar 20, 2023

RossBencina reviewed Mar 20, 2023

View reviewed changes

src/common/pa_front.c Outdated Show resolved Hide resolved

RossBencina changed the title ~~Pa_Initialze(): Guard against recursive calls.~~ Pa_Initialize(): Guard against recursive calls. Mar 20, 2023

daschuer added 4 commits March 21, 2023 02:38

Pa_Initalze(): Guard against recursive calls.

e03753c

This fixes a stack overflow when a diver uses portaudio as well like FlexAsio

Add a debug statement when re-enter Pa_Initalize()

a897650

Add link to issue fixed by the previous commits

079943e

Don't use C99 stdbool.h

9885ecc

daschuer force-pushed the recursive_initalize branch from 65e5b82 to 9885ecc Compare March 21, 2023 01:39

RossBencina reviewed Mar 28, 2023

View reviewed changes

RossBencina assigned daschuer Mar 28, 2023

Use new error code paCanNotInitializeRecursively

9995043

RossBencina reviewed Mar 31, 2023

View reviewed changes

src/common/pa_front.c Outdated Show resolved Hide resolved

Fix error text for paCanNotInitializeRecursively

8faf0f6

RossBencina approved these changes Mar 31, 2023

View reviewed changes

RossBencina requested a review from philburk March 31, 2023 23:17

RossBencina added the final merge review Maintainers flag as ready to make a final merge decision label Mar 31, 2023

MichalPetryka reviewed Apr 2, 2023

View reviewed changes

philburk approved these changes Apr 3, 2023

View reviewed changes

dechamps approved these changes Apr 4, 2023

View reviewed changes

MichalPetryka approved these changes Apr 4, 2023

View reviewed changes

RossBencina merged commit c8b9dd2 into PortAudio:master Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pa_Initialize(): Guard against recursive calls. #794

Pa_Initialize(): Guard against recursive calls. #794

daschuer commented Mar 14, 2023

dechamps commented Mar 14, 2023

daschuer commented Mar 15, 2023

daschuer commented Mar 15, 2023

RossBencina commented Mar 20, 2023

RossBencina Mar 28, 2023

daschuer Mar 28, 2023

RossBencina left a comment

RossBencina commented Mar 31, 2023

dechamps commented Apr 2, 2023

MichalPetryka Apr 2, 2023

daschuer Apr 2, 2023

MichalPetryka Apr 2, 2023

daschuer Apr 3, 2023

philburk Apr 3, 2023

RossBencina Apr 3, 2023

philburk Apr 3, 2023

RossBencina commented Apr 3, 2023

RossBencina commented Apr 3, 2023

RossBencina commented Apr 4, 2023

MichalPetryka commented Apr 4, 2023

Pa_Initialize(): Guard against recursive calls. #794

Pa_Initialize(): Guard against recursive calls. #794

Conversation

daschuer commented Mar 14, 2023

dechamps commented Mar 14, 2023

daschuer commented Mar 15, 2023

daschuer commented Mar 15, 2023

RossBencina commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RossBencina left a comment

Choose a reason for hiding this comment

RossBencina commented Mar 31, 2023

dechamps commented Apr 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RossBencina commented Apr 3, 2023

RossBencina commented Apr 3, 2023

RossBencina commented Apr 4, 2023

MichalPetryka commented Apr 4, 2023