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.

Sign up for GitHub

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

Jump to bottom

Pa_Initialize(): Guard against recursive calls. #794

Merged
merged 6 commits into from
Apr 4, 2023
Merged

Pa_Initialize(): Guard against recursive calls. #794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e03753c
Pa_Initalze(): Guard against recursive calls.
daschuer Mar 14, 2023
a897650
Add a debug statement when re-enter Pa_Initalize()
daschuer Mar 15, 2023
079943e
Add link to issue fixed by the previous commits
daschuer Mar 15, 2023
9885ecc
Don't use C99 stdbool.h
daschuer Mar 15, 2023
9995043
Use new error code paCanNotInitializeRecursively
daschuer Mar 28, 2023
8faf0f6
Fix error text for paCanNotInitializeRecursively
RossBencina Mar 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion include/portaudio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ typedef enum PaErrorCode
paCanNotReadFromAnOutputOnlyStream,
paCanNotWriteToAnInputOnlyStream,
paIncompatibleStreamHostApi,
paBadBufferPtr
paBadBufferPtr,
paCanNotInitializeRecursively
} PaErrorCode;


Expand Down
17 changes: 17 additions & 0 deletions src/common/pa_front.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ static PaUtilHostApiRepresentation **hostApis_ = 0;
static int hostApisCount_ = 0;
static int defaultHostApiIndex_ = 0;
static int initializationCount_ = 0;
static int initializing_ = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

static int deviceCount_ = 0;

PaUtilStreamRepresentation *firstOpenStream_ = NULL;
Expand Down Expand Up @@ -360,8 +361,21 @@ PaError Pa_Initialize( void )
++initializationCount_;
result = paNoError;
}
else if( initializing_ )
{
// a concurrent initialization is already running
RossBencina marked this conversation as resolved.
Show resolved Hide resolved
PA_DEBUG(("Attempting to re-enter Pa_Initialize(), aborting!\n"));
result = paCanNotInitializeRecursively;
}
else
{
// set initializing_ here to
// let recursive calls execute the if branch above.
// This can happen if a driver like FlexAsio itself uses portaudio
// and avoids a stack overflow in the user application.
RossBencina marked this conversation as resolved.
Show resolved Hide resolved
// https://github.com/PortAudio/portaudio/issues/766
initializing_ = 1;

PA_VALIDATE_TYPE_SIZES;
PA_VALIDATE_ENDIANNESS;

Expand All @@ -371,6 +385,8 @@ PaError Pa_Initialize( void )
result = InitializeHostApis();
if( result == paNoError )
++initializationCount_;

initializing_ = 0;
}

PA_LOGAPI_EXIT_PAERROR( "Pa_Initialize", result );
Expand Down Expand Up @@ -453,6 +469,7 @@ const char *Pa_GetErrorText( PaError errorCode )
case paCanNotWriteToAnInputOnlyStream: result = "Can't write to an input only stream"; break;
case paIncompatibleStreamHostApi: result = "Incompatible stream host API"; break;
case paBadBufferPtr: result = "Bad buffer pointer"; break;
case paCanNotInitializeRecursively: result = "PortAudio can not be initialized recursively"; break;
default:
if( errorCode > 0 )
result = "Invalid error code (value greater than zero)";
Expand Down