Race condition during AEC settings modification when sound device is running

[sauwming]

When sound device is running, modifying AEC settings may potentially cause race condition and crash such as:

==37909==ERROR: AddressSanitizer: heap-use-after-free on address 0x61a000057edc at pc 0x000102d38b0c bp 0x00016e1ec560 sp 0x00016e1ec558
    #2 0x000102a77d30 in speex_aec_capture echo_speex.c:305
    #3 0x000102ac0c34 in pjmedia_echo_capture echo_common.c:411
    #4 0x000102b32410 in rec_cb sound_port.c:167

0x61a000057edc is located 604 bytes inside of 1376-byte region [0x61a000057c80,0x61a0000581e0)
freed by thread T4 here:
    #0 0x00010430d480 in free+0x7c (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3d480)
    #1 0x000102d7e0d0 in speex_free os_support.h:81
    #2 0x000102d7dbf0 in speex_preprocess_state_destroy preprocess.c:538
    #3 0x000102a76b14 in speex_aec_destroy echo_speex.c:174
    #4 0x000102ac0188 in pjmedia_echo_destroy echo_common.c:314
    #5 0x000102b30d9c in pjmedia_snd_port_set_ec sound_port.c:776
    #6 0x0001028aa554 in pjsua_set_ec pjsua_aud.c:2476

[nanangizz]

• What if it returns PJ_EPENDING?
• Mention the pending settings in the log as well?

[sauwming]

won't this be considered API change? i.e. it will affect apps that only check for PJ_SUCCESS.

[nanangizz]

Yes, it introduces a behavioral change, but I think it makes sense (perhaps even necessary?) since the settings are no longer applied immediately?

[sauwming]

It's not behavioral change, rather incorrect implementation.

First, are you proposing changing pjmedia only or also pjsua?
For pjsua, it's clear, the doc already specifies the behavior, which is that the settings will be applied for future use if sound device is opened (If software AEC is being used, this function will change the software EC settings. In all cases, the setting will be saved for future opening of the sound device.). So apps have always expected this behavior, and now it's corrected to match the spec. From apps perspective, the behavior stays the same.

For pjmedia however, it's not as clear from the doc. If software AEC is being used, this function will change the software EC settings (no mention when). But it also says not all sound device implementation supports changing the EC setting once the device has been opened. So apps should already know that changing the EC setting may not be immediate. But yes, it's not as explicitly stated as pjsua.

So if you want to change pjmedia only, perhaps it's still acceptable, except that it will introduce a difference between the pjsua and pjmedia APIs.

[nanangizz]

As I understand it, in pjsua, the intention is to apply the settings immediately whenever possible (for EC & other settings, see also pjsua_snd_set_setting() docs). The "future use" is an additional feature that preserves the settings.

Btw, the sound port API does not seem to have start/stop functions, only create & destroy, so what does "future use"/keeping-settings mean/purpose?

[sauwming]

Very good point.
I try a different approach in the last commit, by stopping the audio stream first.