Fix a data race in AudioEncoderMutableImpl and derived classes

Fix a data race in AudioEncoderMutableImpl and derived classes

Before this change, it could happen that a caller would get a pointer
to the encoder_ but not use it before another thread called the
Reconstruct method, changing the pointer. This of course resulted in
bad access crashes. With this change, each use of the pointer acquired
from the encoder() method is protected by the same lock that is
required to update the pointer. Note that this fix is probably too
aggressive, since it also affects the Opus implementation; the crash
has so far only been seen for iSAC.

Also adding a test to trigger the problem. The test did not trigger
the problem deterministically, but out would typically find it in less
than 1000 runs.

BUG=chromium:499468
R=jmarusic@webrtc.org, kwiberg@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/a6aa6d96f8aa10736c76deb2a6ef09027d375a4a

[hlundin-webrtc, 2015-06-12 13:21:09]

henrik.lundin@webrtc.org changed reviewers:
+ jmarusic@webrtc.org, kwiberg@webrtc.org

[hlundin-webrtc, 2015-06-12 13:21:09]

Jelena, Karl,

Please, review this CL.

Thanks!

[Jelena, 2015-06-12 15:06:45]

Didn't have time to look at the test, but the fix is good. LGTM

[Jelena, 2015-06-13 09:47:50]

https://codereview.webrtc.org/1176303004/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h (right):

https://codereview.webrtc.org/1176303004/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h:97:
CriticalSectionScoped cs(encoder_lock_.get());
Actually, lock needs to protect the config_ as well, so move it one line up.

https://codereview.webrtc.org/1176303004/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h:102: const
typename T::Config& config() const { return config_; }
Since config_ can be modified on another thread, you need to obtain a lock
first. For the same reason, it is unsafe to return a reference, so this method
must return a copy of config_.

[kwiberg-webrtc, 2015-06-13 20:10:54]

> Fix a data race in AudioEncoderMutableImpl and derived classes
>
> Before this change, it could happen that a caller would get a
> pointer to the encoder_ but not use it before another thread called
> the Reconstruct method, changing the pointer. This of course
> resulted in bad access crashes. With this change, each use of the
> pointer acquired from the encoder() method is protected by the same
> lock that is required to update the pointer. Note that this fix is
> probably too aggressive, since it also affects the Opus
> implementation; the crash has so far only been seen for iSAC.

Hmm. Am I wrong to assume that the crash can only possibly affect
iSAC, since encoders and decoders are each accessed by a single
thread?---the only reason we get a race is that the iSAC encoder and
decoder is the same object. The problem would then be something like
the AudioDecoder implementation not coping with the
AudioEncoderMutable implementation reconstructing the codec instance.

If so, this is a very ugly and overkill way to solve the problem,
since it affects all other codecs too just because they happen to
share code with iSAC. But it only needs to fill the gap until my CL
which splits iSAC into separate encoder and decoder objects can land,
at which point all our troubles will be over.

[kwiberg-webrtc, 2015-06-13 20:27:49]

lgtm

https://codereview.webrtc.org/1176303004/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h (right):

https://codereview.webrtc.org/1176303004/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h:97:
CriticalSectionScoped cs(encoder_lock_.get());
On 2015/06/13 09:47:50, Jelena wrote:
> Actually, lock needs to protect the config_ as well, so move it one line up.

I don't think it does, actually. If I'm right, all AudioEncoder(Mutable)
accesses are from one thread, and all AudioDecoder accesses are from one thread,
so the only possible race is between those two threads. And the decoder thread
will never touch config_.

That said, since this is a stopgap solution, I'm not at all opposed to following
your advice. Just make config_ GUARDED_BY(encoder_lock_) in that case.

https://codereview.webrtc.org/1176303004/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h:102: const
typename T::Config& config() const { return config_; }
On 2015/06/13 09:47:50, Jelena wrote:
> Since config_ can be modified on another thread, you need to obtain a lock
> first. For the same reason, it is unsafe to return a reference, so this method
> must return a copy of config_.

Same comment as on line 97.

[Jelena, 2015-06-15 07:37:35]

On 2015/06/13 20:27:49, kwiberg1 wrote:
> lgtm
> 
>
https://codereview.webrtc.org/1176303004/diff/1/webrtc/modules/audio_coding/c...
> File webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h (right):
> 
>
https://codereview.webrtc.org/1176303004/diff/1/webrtc/modules/audio_coding/c...
> webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h:97:
> CriticalSectionScoped cs(encoder_lock_.get());
> On 2015/06/13 09:47:50, Jelena wrote:
> > Actually, lock needs to protect the config_ as well, so move it one line up.
> 
> I don't think it does, actually. If I'm right, all AudioEncoder(Mutable)
> accesses are from one thread, and all AudioDecoder accesses are from one
thread,
> so the only possible race is between those two threads. And the decoder thread
> will never touch config_.
> 
> That said, since this is a stopgap solution, I'm not at all opposed to
following
> your advice. Just make config_ GUARDED_BY(encoder_lock_) in that case.
> 
>
https://codereview.webrtc.org/1176303004/diff/1/webrtc/modules/audio_coding/c...
> webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h:102: const
> typename T::Config& config() const { return config_; }
> On 2015/06/13 09:47:50, Jelena wrote:
> > Since config_ can be modified on another thread, you need to obtain a lock
> > first. For the same reason, it is unsafe to return a reference, so this
method
> > must return a copy of config_.
> 
> Same comment as on line 97.

Reconstruct was called on another thread (or there wouldn't have been a crash
like this), so any data that is modified in it needs protection (at least at
this moment).

[hlundin-webrtc, 2015-06-15 09:31:51]

Including config_ in the critical section

[hlundin-webrtc, 2015-06-15 09:37:23]

Return config_ by value instead of reference

[hlundin-webrtc, 2015-06-15 09:38:10]

Thanks for the comments. Please, see new patch set before I commit.

https://codereview.webrtc.org/1176303004/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h (right):

https://codereview.webrtc.org/1176303004/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h:97:
CriticalSectionScoped cs(encoder_lock_.get());
On 2015/06/13 20:27:49, kwiberg1 wrote:
> On 2015/06/13 09:47:50, Jelena wrote:
> > Actually, lock needs to protect the config_ as well, so move it one line up.
> 
> I don't think it does, actually. If I'm right, all AudioEncoder(Mutable)
> accesses are from one thread, and all AudioDecoder accesses are from one
thread,
> so the only possible race is between those two threads. And the decoder thread
> will never touch config_.
> 
> That said, since this is a stopgap solution, I'm not at all opposed to
following
> your advice. Just make config_ GUARDED_BY(encoder_lock_) in that case.

Moved the critsect as suggested, and added GUARDED_BY. However, the latter
caused some collateral damage.

https://codereview.webrtc.org/1176303004/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h:102: const
typename T::Config& config() const { return config_; }
On 2015/06/13 09:47:50, Jelena wrote:
> Since config_ can be modified on another thread, you need to obtain a lock
> first. For the same reason, it is unsafe to return a reference, so this method
> must return a copy of config_.

Done.

[kwiberg-webrtc, 2015-06-15 11:12:19]

lgtm

[Jelena, 2015-06-15 11:34:27]

lgtm

[hlundin-webrtc, 2015-06-15 11:46:30]

Committed patchset #4 (id:60001) manually as
a6aa6d96f8aa10736c76deb2a6ef09027d375a4a (presubmit successful).