807254 - WebRTC data race with webrtc::AudioDeviceLinuxALSA::PlayoutWarning vs. webrtc::AudioDeviceLinuxALSA::Init

Status: RESOLVED INVALID

(Core :: WebRTC: Audio/Video, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

media/webrtc/trunk/src/modules/audio_device/main/source/audio_device_impl.cc:492
[...]
    // kPlayoutWarning
    if (_ptrAudioDevice->PlayoutWarning())
    {
        CriticalSectionScoped lock(&_critSectEventCb);
        if (_ptrCbAudioDeviceObserver)
        {
            WEBRTC_TRACE(kTraceWarning, kTraceAudioDevice, _id, "=> OnWarningIsReported(kPlayoutWarning)");
            _ptrCbAudioDeviceObserver->OnWarningIsReported(AudioDeviceObserver::kPlayoutWarning);
        }
        _ptrAudioDevice->ClearPlayoutWarning();
    }
[...]

media/webrtc/trunk/src/modules/audio_device/main/source/linux/audio_device_alsa_linux.cc:185
WebRtc_Word32 AudioDeviceLinuxALSA::Init()
{

    CriticalSectionScoped lock(&_critSect);
[...]

Tested with m-c changeset: 111684:e19e170d2f6d

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

I don't understand how this could be anything except a false positive.  I don't see how _critSect can conflict with any vars of the other side.

Comment 2

[Christoph Diehl [:posidron]]

I am not familiar with the code but the documentation says that if locks are in use and the race was found in pure happens-before mode than it's not a false positive.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

"locks are not involved" - what does this mean exactly?  this code is explicitly using critical section locks (though different ones), so these accesses are under lock.

Comment 4

[Christoph Diehl [:posidron]]

I did not say, if locks are *not* involved. I said, if locks are involved. :)

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Aha.  audio_device_alsa_linux.cc:185 *for that changeset* is this:

    _playWarning = 0;

and media/webrtc/trunk/src/modules/audio_device/main/source/audio_device_impl.cc:492 is

    // kPlayoutWarning
    if (_ptrAudioDevice->PlayoutWarning())
    {

That makes far more sense.  The MXR snippets were confused by checkins since the changeset was pulled.

These first is the init routine, so Process() can't be called before init has been (and appropriate interlocks occur), so this is a false positive.  You should probably mark it as such.  Thanks!