VideoCaptureManager: fixed double-deletion bug in StopCaptureForClient() on error

VideoCaptureManager: fixed double-deletion bug in StopCaptureForClient() on error

VideoCaptureManager::StopCaptureForClient() [1], when called in error
condition, will run, in the beginning,
  DeviceEntry* entry = GetDeviceEntryByController(controller);
but then, if |aborted_due_to_error|, it would also call (l.719 [2])
synchronuosly:

 listener_->Aborted(...);
  MediaStreamManager::StopDevice(...)
   MediaStreamManager::CloseDevice(...);
    VideoCaptureManager::Close(...);
     VideoCaptureManager::DestroyDeviceIfNoClient(...);

the last one will invalidate |entry| and destroy the |controller|
needed in l.727 [3], hence the UAF.

This bug was there before the blamed CLs (see bug), but the
migration to mojo has made this path more possible for Cluster
Fuzz to exercise. Note: I have no clue how ClusterFuzz managed
this, I had to patch the code to cause this error condition :)

[1] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_manager.cc?q=VideoCaptureManager::StopCaptureForClient&sq=package:chromium&l=689&dr=CSs
[2] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_manager.cc?q=VideoCaptureManager::StopCaptureForClient&sq=package:chromium&dr=CSs&l=719
[3] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_manager.cc?q=VideoCaptureManager::StopCaptureForClient&sq=package:chromium&dr=CSs&l=727

BUG=654199
TEST= see bug for repro; doesn't crash with the patch, capture
device is still correctly closed.

Committed: https://crrev.com/abb68a36070c6f5a183c8db994a3f1f9e250d16f
Cr-Commit-Position: refs/heads/master@{#425525}

[mcasas, 2016-10-14 21:43:00]

Description was changed from

==========
VideoCaptureManager: fixed double-deletion bug in StopCaptureForClient() on
error

VideoCaptureManager::StopCaptureForClient() [1], when called in error
condition, will run, in the beginning,
  DeviceEntry* entry = GetDeviceEntryByController(controller);
but then, if |aborted_due_to_error|, it would also call (l.719)
synchronuosly:

 listener_->Aborted(...);
  MediaStreamManager::StopDevice(...)
   MediaStreamManager::CloseDevice(...);
    VideoCaptureManager::Close(...);
     VideoCaptureManager::DestroyDeviceIfNoClient(...);

the last one will invalidate |entry| and destroy the |controller|
needed in l.727, hence the UAF.

This bug was there before the blamed CLs (see bug), but the
migration to mojo has made this path more possible for Cluster
Fuzz to exercise. Note: I have no clue how ClusterFuzz managed
this, I had to patch the code to cause this error condition :)


[1]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...

BUG=654199
TEST= see bug for repro; doesn't crash with the patch, capture
device is still correctly closed.
==========

to

==========
VideoCaptureManager: fixed double-deletion bug in StopCaptureForClient() on
error

VideoCaptureManager::StopCaptureForClient() [1], when called in error
condition, will run, in the beginning,
  DeviceEntry* entry = GetDeviceEntryByController(controller);
but then, if |aborted_due_to_error|, it would also call (l.719 [2])
synchronuosly:

 listener_->Aborted(...);
  MediaStreamManager::StopDevice(...)
   MediaStreamManager::CloseDevice(...);
    VideoCaptureManager::Close(...);
     VideoCaptureManager::DestroyDeviceIfNoClient(...);

the last one will invalidate |entry| and destroy the |controller|
needed in l.727 [3], hence the UAF.

This bug was there before the blamed CLs (see bug), but the
migration to mojo has made this path more possible for Cluster
Fuzz to exercise. Note: I have no clue how ClusterFuzz managed
this, I had to patch the code to cause this error condition :)


[1]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...
[2]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...
[3]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...

BUG=654199
TEST= see bug for repro; doesn't crash with the patch, capture
device is still correctly closed.
==========

[mcasas, 2016-10-14 21:53:35]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-10-14 22:04:48]

mcasas@chromium.org changed reviewers:
+ xianglu@chromium.org

[mcasas, 2016-10-14 22:04:48]

xianglu@ PTAL

[xianglu, 2016-10-14 23:28:22]

https://codereview.chromium.org/2417383002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2417383002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:717: // Aborted()
call might synchronously destroy |entry|, recheck.
If the entry is always going to be deleted after Aborted() on the same thread,
why do we need to get and check again? Can we safely return?

[mcasas, 2016-10-14 23:44:10]

https://codereview.chromium.org/2417383002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/2417383002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:717: // Aborted()
call might synchronously destroy |entry|, recheck.
On 2016/10/14 23:28:22, xianglu wrote:
> If the entry is always going to be deleted after Aborted() on the same thread,
> why do we need to get and check again? Can we safely return?

The |entry| might still be alive if there's more than
one client using it (e.g. two tabs capturing using the
same webcam) -- that's why the method below is
called DestroyDeviceEntryIfNoClients(). Or that's 
my understanding, since this class and associated
fellows are very complex to reason about IMHO.

[xianglu, 2016-10-14 23:57:53]

lgtm

[mcasas, 2016-10-15 00:10:30]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-10-15 00:10:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-15 01:08:17]

Description was changed from

==========
VideoCaptureManager: fixed double-deletion bug in StopCaptureForClient() on
error

VideoCaptureManager::StopCaptureForClient() [1], when called in error
condition, will run, in the beginning,
  DeviceEntry* entry = GetDeviceEntryByController(controller);
but then, if |aborted_due_to_error|, it would also call (l.719 [2])
synchronuosly:

 listener_->Aborted(...);
  MediaStreamManager::StopDevice(...)
   MediaStreamManager::CloseDevice(...);
    VideoCaptureManager::Close(...);
     VideoCaptureManager::DestroyDeviceIfNoClient(...);

the last one will invalidate |entry| and destroy the |controller|
needed in l.727 [3], hence the UAF.

This bug was there before the blamed CLs (see bug), but the
migration to mojo has made this path more possible for Cluster
Fuzz to exercise. Note: I have no clue how ClusterFuzz managed
this, I had to patch the code to cause this error condition :)


[1]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...
[2]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...
[3]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...

BUG=654199
TEST= see bug for repro; doesn't crash with the patch, capture
device is still correctly closed.
==========

to

==========
VideoCaptureManager: fixed double-deletion bug in StopCaptureForClient() on
error

VideoCaptureManager::StopCaptureForClient() [1], when called in error
condition, will run, in the beginning,
  DeviceEntry* entry = GetDeviceEntryByController(controller);
but then, if |aborted_due_to_error|, it would also call (l.719 [2])
synchronuosly:

 listener_->Aborted(...);
  MediaStreamManager::StopDevice(...)
   MediaStreamManager::CloseDevice(...);
    VideoCaptureManager::Close(...);
     VideoCaptureManager::DestroyDeviceIfNoClient(...);

the last one will invalidate |entry| and destroy the |controller|
needed in l.727 [3], hence the UAF.

This bug was there before the blamed CLs (see bug), but the
migration to mojo has made this path more possible for Cluster
Fuzz to exercise. Note: I have no clue how ClusterFuzz managed
this, I had to patch the code to cause this error condition :)


[1]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...
[2]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...
[3]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...

BUG=654199
TEST= see bug for repro; doesn't crash with the patch, capture
device is still correctly closed.
==========

[commit-bot: I haz the power, 2016-10-15 01:08:18]

Committed patchset #1 (id:20001)

[commit-bot: I haz the power, 2016-10-15 01:10:46]

Description was changed from

==========
VideoCaptureManager: fixed double-deletion bug in StopCaptureForClient() on
error

VideoCaptureManager::StopCaptureForClient() [1], when called in error
condition, will run, in the beginning,
  DeviceEntry* entry = GetDeviceEntryByController(controller);
but then, if |aborted_due_to_error|, it would also call (l.719 [2])
synchronuosly:

 listener_->Aborted(...);
  MediaStreamManager::StopDevice(...)
   MediaStreamManager::CloseDevice(...);
    VideoCaptureManager::Close(...);
     VideoCaptureManager::DestroyDeviceIfNoClient(...);

the last one will invalidate |entry| and destroy the |controller|
needed in l.727 [3], hence the UAF.

This bug was there before the blamed CLs (see bug), but the
migration to mojo has made this path more possible for Cluster
Fuzz to exercise. Note: I have no clue how ClusterFuzz managed
this, I had to patch the code to cause this error condition :)


[1]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...
[2]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...
[3]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...

BUG=654199
TEST= see bug for repro; doesn't crash with the patch, capture
device is still correctly closed.
==========

to

==========
VideoCaptureManager: fixed double-deletion bug in StopCaptureForClient() on
error

VideoCaptureManager::StopCaptureForClient() [1], when called in error
condition, will run, in the beginning,
  DeviceEntry* entry = GetDeviceEntryByController(controller);
but then, if |aborted_due_to_error|, it would also call (l.719 [2])
synchronuosly:

 listener_->Aborted(...);
  MediaStreamManager::StopDevice(...)
   MediaStreamManager::CloseDevice(...);
    VideoCaptureManager::Close(...);
     VideoCaptureManager::DestroyDeviceIfNoClient(...);

the last one will invalidate |entry| and destroy the |controller|
needed in l.727 [3], hence the UAF.

This bug was there before the blamed CLs (see bug), but the
migration to mojo has made this path more possible for Cluster
Fuzz to exercise. Note: I have no clue how ClusterFuzz managed
this, I had to patch the code to cause this error condition :)

[1]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...
[2]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...
[3]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/vide...

BUG=654199
TEST= see bug for repro; doesn't crash with the patch, capture
device is still correctly closed.

Committed: https://crrev.com/abb68a36070c6f5a183c8db994a3f1f9e250d16f
Cr-Commit-Position: refs/heads/master@{#425525}
==========

[commit-bot: I haz the power, 2016-10-15 01:10:47]

Patchset 1 (id:??) landed as
https://crrev.com/abb68a36070c6f5a183c8db994a3f1f9e250d16f
Cr-Commit-Position: refs/heads/master@{#425525}