PortAudio / portaudio

PortAudio is a cross-platform, open-source C language library for real-time audio input and output.
Other
1.49k stars 307 forks source link

Pa_Initialize(): Guard against recursive calls. #794

Closed daschuer closed 1 year ago

daschuer commented 1 year ago

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

Fixes: https://github.com/PortAudio/portaudio/issues/766

dechamps commented 1 year ago

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 commented 1 year ago

Done

daschuer commented 1 year ago

Done.

RossBencina commented 1 year ago

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:

https://github.com/PortAudio/portaudio/blob/65e5b822d3018f2857f6f48b50d9693f98e0ace2/include/portaudio.h#L155

With a new error text case added here:

https://github.com/PortAudio/portaudio/blob/ed922d949c34402a815231a2fc5146a8b4c024c9/src/common/pa_front.c#L456

RossBencina commented 1 year ago

@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 commented 1 year ago

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.)

RossBencina commented 1 year ago

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

RossBencina commented 1 year ago

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 commented 1 year ago

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

MichalPetryka commented 1 year ago

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

Yeah, it does.