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.

Review-Url: https://codereview.chromium.org/2417383002
Cr-Commit-Position: refs/heads/master@{#425525}
(cherry picked from commit abb68a36070c6f5a183c8db994a3f1f9e250d16f)


NOTRY=true
NOPRESUBMIT=true

[mcasas, 2016-10-25 21:17:29]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-10-25 21:18:21]

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-25 21:18:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 21:18:22]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[mcasas, 2016-10-25 21:25:00]

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

[mcasas, 2016-10-25 21:25:00]

emircan@ RS plz

[emircan, 2016-10-25 22:05:52]

lgtm

[mcasas, 2016-10-25 22:07:37]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-10-25 22:08:26]

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-25 22:08:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 22:08:28]

CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true
and NOPRESUBMIT=true in order for the CQ to process them

[mcasas, 2016-10-25 22:12:11]

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.

Review-Url: https://codereview.chromium.org/2417383002
Cr-Commit-Position: refs/heads/master@{#425525}
(cherry picked from commit abb68a36070c6f5a183c8db994a3f1f9e250d16f)
==========

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.

Review-Url: https://codereview.chromium.org/2417383002
Cr-Commit-Position: refs/heads/master@{#425525}
(cherry picked from commit abb68a36070c6f5a183c8db994a3f1f9e250d16f)


NOTRY=true
NOPRESUBMIT=true
==========

[mcasas, 2016-10-25 22:12:35]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-10-25 22:13:24]

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-25 22:17:48]

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.

Review-Url: https://codereview.chromium.org/2417383002
Cr-Commit-Position: refs/heads/master@{#425525}
(cherry picked from commit abb68a36070c6f5a183c8db994a3f1f9e250d16f)


NOTRY=true
NOPRESUBMIT=true
==========

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.

Review-Url: https://codereview.chromium.org/2417383002
Cr-Commit-Position: refs/heads/master@{#425525}
(cherry picked from commit abb68a36070c6f5a183c8db994a3f1f9e250d16f)


NOTRY=true
NOPRESUBMIT=true
==========

[commit-bot: I haz the power, 2016-10-25 22:17:50]

Committed patchset #1 (id:1)