HasRefCountedTypeAsRawPtr doesn't work correctly when binding a raw pointer lvalue |
||||
Issue descriptionI'm not quite sure all the exact conditions of this bug, but it appears that binding a raw pointer variable (to a ref counted type) is sufficient to bypass this check =( Wild guess is that std::is_pointer<T> unexpectedly isn't returning true in NeedsScopedRefptrButGetsRawPtr, but I haven't narrowed that down yet. https://chromium-review.googlesource.com/549816 is an example that demonstrates this.
,
Jun 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/65de71019e3c4a90afbc3ff7a391301959816031 commit 65de71019e3c4a90afbc3ff7a391301959816031 Author: tzik <tzik@chromium.org> Date: Tue Jun 27 08:05:53 2017 Avoid binding raw pointers to ref-counted types in //content base::Bind should have reject raw pointers to ref-counted types, but the compile-time check has been regressed for a while. This CL removes the banned pattern introduced while the check is not working. Bug: 737010 Change-Id: Ieb0ee10c0a8afca40fa2fd6a26306cfbbd84e4b5 Reviewed-on: https://chromium-review.googlesource.com/549520 Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#482570} [modify] https://crrev.com/65de71019e3c4a90afbc3ff7a391301959816031/content/browser/devtools/protocol/storage_handler.cc [modify] https://crrev.com/65de71019e3c4a90afbc3ff7a391301959816031/content/browser/loader/resource_dispatcher_host_impl.cc [modify] https://crrev.com/65de71019e3c4a90afbc3ff7a391301959816031/content/browser/media/media_interface_proxy.cc [modify] https://crrev.com/65de71019e3c4a90afbc3ff7a391301959816031/content/browser/service_worker/service_worker_dispatcher_host_unittest.cc [modify] https://crrev.com/65de71019e3c4a90afbc3ff7a391301959816031/content/browser/service_worker/service_worker_url_loader_job.cc [modify] https://crrev.com/65de71019e3c4a90afbc3ff7a391301959816031/content/browser/service_worker/service_worker_url_request_job.cc [modify] https://crrev.com/65de71019e3c4a90afbc3ff7a391301959816031/content/renderer/input/main_thread_event_queue.cc [modify] https://crrev.com/65de71019e3c4a90afbc3ff7a391301959816031/content/renderer/media/audio_renderer_sink_cache_unittest.cc
,
Jun 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/81b8c487cc59b6c0c79fb83f7b156daa2be045ca commit 81b8c487cc59b6c0c79fb83f7b156daa2be045ca Author: Sunny Sachanandani <sunnyps@chromium.org> Date: Wed Jun 28 00:10:50 2017 gpu: Fix message filter use after free. base::Bind should static_assert that raw pointers are not passed as parameters but that broke a while ago. This change adds base::RetainedRef to the message filter pointer so that it is retained by the closure. An alternative would be to use scoped_refptr instead of raw pointer as AddFilter's parameter but I wanted to keep it consistent with IPC::ChannelProxy. More context can be found in the following chromium-dev thread: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/qH5swrLHPmg/discussion R=piman BUG=729483, 737010 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ief8716ba003283ded41a2efe991c67966b72c7d2 Reviewed-on: https://chromium-review.googlesource.com/550581 Reviewed-by: Antoine Labour <piman@chromium.org> Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org> Cr-Commit-Position: refs/heads/master@{#482807} [modify] https://crrev.com/81b8c487cc59b6c0c79fb83f7b156daa2be045ca/gpu/ipc/service/gpu_channel.cc
,
Jun 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/df97f3d789aabfdbe74d381b28d09e037453d6bd commit df97f3d789aabfdbe74d381b28d09e037453d6bd Author: tzik <tzik@chromium.org> Date: Wed Jun 28 18:34:42 2017 Retain ref-count of a bound ref-counted object in //printing |data| at PrintedDocument::DebugDumpData argument is a ref-counted type, and an explicit lifetime management is needed on base::Bind. This pattern of raw pointers to a ref-counted object should be checked in base::Bind implementation, but it has failed to ban the pattern when the pointee was const. Bug: 737010 Change-Id: Icf0b08418e6fcda2b88c61db32c25d88703362cf Reviewed-on: https://chromium-review.googlesource.com/551437 Commit-Queue: Lei Zhang <thestig@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Cr-Commit-Position: refs/heads/master@{#483076} [modify] https://crrev.com/df97f3d789aabfdbe74d381b28d09e037453d6bd/printing/printed_document.cc
,
Jun 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/12dd20d1972b5a0a21003555fdababb37f2f747e commit 12dd20d1972b5a0a21003555fdababb37f2f747e Author: tzik <tzik@chromium.org> Date: Fri Jun 30 19:38:16 2017 Retain ref-count of a bound ref-counted object in //net disk_cache::SimpleEntryImpl and net::SSLCertRequestInfo are ref-counte types, and they needs explicit lifetime management when they are passed to base::Bind. This pattern of raw pointers to a ref-counted object should be checked in base::Bind implementation, but it has failed to ban the pattern when the pointee was const. In GetClientCerts, the lifetime of net::SSLCertRequestInfo is managed by the caller, so just Unretained() should work. For SimpleEntryImpl stuff, the resulting callbacks are consumed soon, so Unretained() should work there too. Bug: 737010 Change-Id: I3a74d10636b521efc081804939bb789215b0679f Reviewed-on: https://chromium-review.googlesource.com/551698 Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Matt Mueller <mattm@chromium.org> Commit-Queue: Matt Mueller <mattm@chromium.org> Cr-Commit-Position: refs/heads/master@{#483790} [modify] https://crrev.com/12dd20d1972b5a0a21003555fdababb37f2f747e/chrome/browser/chromeos/net/client_cert_store_chromeos.cc [modify] https://crrev.com/12dd20d1972b5a0a21003555fdababb37f2f747e/net/disk_cache/simple/simple_net_log_parameters.cc [modify] https://crrev.com/12dd20d1972b5a0a21003555fdababb37f2f747e/net/ssl/client_cert_store_nss.cc
,
Jul 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7667b3ef65ac14e6f537cb8bc5fc95dbc466650d commit 7667b3ef65ac14e6f537cb8bc5fc95dbc466650d Author: tzik <tzik@chromium.org> Date: Mon Jul 03 04:44:06 2017 Retain ref-count of a bound ref-counted object around Extension extensions::Extension is a ref-counted type, and when it is passed to base::Bind, an explicit lifetime management is needed. This pattern of raw pointers to a ref-counted object should be checked in base::Bind implementation, but it has failed to ban the pattern when the pointee was const. TBR=pkotwicz@chromium.org Bug: 737010 Change-Id: Ic98d630e43230e6b7c556313c5d64e655219e924 Reviewed-on: https://chromium-review.googlesource.com/552217 Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Reviewed-by: Drew Wilson <atwilson@chromium.org> Reviewed-by: Sergey Ulanov <sergeyu@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#483941} [modify] https://crrev.com/7667b3ef65ac14e6f537cb8bc5fc95dbc466650d/chrome/browser/background/background_mode_manager.cc [modify] https://crrev.com/7667b3ef65ac14e6f537cb8bc5fc95dbc466650d/chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc [modify] https://crrev.com/7667b3ef65ac14e6f537cb8bc5fc95dbc466650d/chrome/browser/extensions/chrome_mojo_service_registration.cc [modify] https://crrev.com/7667b3ef65ac14e6f537cb8bc5fc95dbc466650d/chrome/browser/extensions/extension_disabled_ui.cc [modify] https://crrev.com/7667b3ef65ac14e6f537cb8bc5fc95dbc466650d/chrome/browser/media/public_session_media_access_handler.cc [modify] https://crrev.com/7667b3ef65ac14e6f537cb8bc5fc95dbc466650d/chrome/browser/media/public_session_tab_capture_access_handler.cc [modify] https://crrev.com/7667b3ef65ac14e6f537cb8bc5fc95dbc466650d/chrome/browser/media_galleries/fileapi/media_file_system_backend.cc [modify] https://crrev.com/7667b3ef65ac14e6f537cb8bc5fc95dbc466650d/chrome/browser/themes/theme_service.cc [modify] https://crrev.com/7667b3ef65ac14e6f537cb8bc5fc95dbc466650d/extensions/browser/mojo/service_registration.cc [modify] https://crrev.com/7667b3ef65ac14e6f537cb8bc5fc95dbc466650d/extensions/common/features/simple_feature.cc
,
Jul 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3d404f0e5607b08202261edfbaf0174069510667 commit 3d404f0e5607b08202261edfbaf0174069510667 Author: tzik <tzik@chromium.org> Date: Mon Jul 03 04:59:59 2017 Retain a reference to |cast_socket_service| in cast_channel_apitest.cc CastSocketService is a ref counted type, and its raw pointer will be banned on base::Bind soon as an error prone pattern. This CL adds base::RetainedRef to specify the lifetime management explicitly. TBR=mfoltz@chromium.org Bug: 737010 Change-Id: I55fd187231dc4515ba7b5ab1a56ca1c4b5357d21 Reviewed-on: https://chromium-review.googlesource.com/557585 Reviewed-by: Taiju Tsuiki <tzik@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#483942} [modify] https://crrev.com/3d404f0e5607b08202261edfbaf0174069510667/extensions/browser/api/cast_channel/cast_channel_apitest.cc
,
Jul 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2ac521d3b777d6cc3311a8c02dce5c93e4cb9a7a commit 2ac521d3b777d6cc3311a8c02dce5c93e4cb9a7a Author: tzik <tzik@chromium.org> Date: Mon Jul 03 06:07:49 2017 Avoid binding raw pointers to ref-counted objects on base::Bind Bug: 737010 Change-Id: I91fe66a33bf4a31321cbf3cd67bf60ec190baf1d Reviewed-on: https://chromium-review.googlesource.com/558748 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#483952} [modify] https://crrev.com/2ac521d3b777d6cc3311a8c02dce5c93e4cb9a7a/content/public/test/service_worker_test_helpers.cc
,
Jul 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1d692a2e0d1b58e02760c9c94293a77ecb477440 commit 1d692a2e0d1b58e02760c9c94293a77ecb477440 Author: tzik <tzik@chromium.org> Date: Mon Jul 03 11:01:26 2017 Ban raw pointers to ref-counted types on base::Bind base::Bind is intended to reject raw pointers to ref-counted types as it's likely a bug. However, it has been regressed and failed to reject the problematic pointers when it's passed as lvalue reference. HasRefCountedTypeAsRawPtr requires a non reference type to check the assertion statement, (e.g. requires `T*` rather than `T*&`). In MakeBindStateTypeImpl in bind_internals.h, when a pointer type is passed to base::Bind as an rvalue reference (e.g. `T*&&`), corresponding BoundArgs item is a non-reference pointer type (e.g. `T*`) that passes the unittest, however when it passed as an lvalue reference (e.g. `T*&`), corresponding BoundArgs item is a reference to the pointer (e.g. `T*&`). That causes the failure, and was not tested. This CL fixes the compile-time assertion and fixes all banned usage of base::Bind. Bug: 737010 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ic3293746c22762f7900375b2cbb29ed5363a2d00 Reviewed-on: https://chromium-review.googlesource.com/549537 Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Reviewed-by: Sami Kyöstilä <skyostil@chromium.org> Reviewed-by: Yuzhu Shen <yzshen@chromium.org> Reviewed-by: danakj <danakj@chromium.org> Reviewed-by: Tom Sepez <tsepez@chromium.org> Reviewed-by: Luke Halliwell <halliwell@chromium.org> Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Reviewed-by: Raymes Khoury <raymes@chromium.org> Reviewed-by: Bernhard Bauer <bauerb@chromium.org> Cr-Commit-Position: refs/heads/master@{#483983} [modify] https://crrev.com/1d692a2e0d1b58e02760c9c94293a77ecb477440/base/bind_internal.h [modify] https://crrev.com/1d692a2e0d1b58e02760c9c94293a77ecb477440/base/bind_unittest.nc [modify] https://crrev.com/1d692a2e0d1b58e02760c9c94293a77ecb477440/base/memory/raw_scoped_refptr_mismatch_checker.h [modify] https://crrev.com/1d692a2e0d1b58e02760c9c94293a77ecb477440/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/1d692a2e0d1b58e02760c9c94293a77ecb477440/chrome/browser/engagement/site_engagement_service_unittest.cc [modify] https://crrev.com/1d692a2e0d1b58e02760c9c94293a77ecb477440/chrome/browser/profiles/profile_browsertest.cc [modify] https://crrev.com/1d692a2e0d1b58e02760c9c94293a77ecb477440/chromecast/net/connectivity_checker_impl.cc [modify] https://crrev.com/1d692a2e0d1b58e02760c9c94293a77ecb477440/ipc/ipc_mojo_bootstrap.cc [modify] https://crrev.com/1d692a2e0d1b58e02760c9c94293a77ecb477440/media/gpu/ipc/service/gpu_jpeg_decode_accelerator_unittest.cc [modify] https://crrev.com/1d692a2e0d1b58e02760c9c94293a77ecb477440/mojo/edk/system/node_controller.cc [modify] https://crrev.com/1d692a2e0d1b58e02760c9c94293a77ecb477440/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
,
Jul 3 2017
,
Jul 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4a8a30378c81e16745d066d3eacc8d939c392996 commit 4a8a30378c81e16745d066d3eacc8d939c392996 Author: Sunny Sachanandani <sunnyps@chromium.org> Date: Wed Jul 05 23:24:08 2017 gpu: Fix message filter use after free. base::Bind should static_assert that raw pointers are not passed as parameters but that broke a while ago. This change adds base::RetainedRef to the message filter pointer so that it is retained by the closure. An alternative would be to use scoped_refptr instead of raw pointer as AddFilter's parameter but I wanted to keep it consistent with IPC::ChannelProxy. More context can be found in the following chromium-dev thread: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/qH5swrLHPmg/discussion R=piman TBR=sunnyps@chromium.org BUG=729483, 737010 (cherry picked from commit 81b8c487cc59b6c0c79fb83f7b156daa2be045ca) Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: Ief8716ba003283ded41a2efe991c67966b72c7d2 Reviewed-on: https://chromium-review.googlesource.com/550581 Reviewed-by: Antoine Labour <piman@chromium.org> Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#482807} Reviewed-on: https://chromium-review.googlesource.com/560776 Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Cr-Commit-Position: refs/branch-heads/3112@{#524} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/4a8a30378c81e16745d066d3eacc8d939c392996/gpu/ipc/service/gpu_channel.cc
,
Jul 5 2017
Should some (all?) of these be merged to M60? I'm asking because this bug was found due to a gpu process crash (see merge in #11) and I suspect there may be other crashes like this.
,
Jul 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9805c860a67407267ff9c3517c29aacf2238a014 commit 9805c860a67407267ff9c3517c29aacf2238a014 Author: tzik <tzik@chromium.org> Date: Thu Jul 06 00:15:51 2017 Remove dummy AddRef/Release from //ipc/ipc_sync_channel_unittest.cc `Worker` class in ipc_sync_channel_unittest.cc has a dummy AddRef() and Release() to pretend a ref-counted object. This type of objects are confusing on base:::Bind. E.g., the object is wrapped with scoped_refptr if it's passed as a receiver, OTOH, it's unretained if it's passed as a non-receiver parameter. This CL removes the dummy AddRef()/Release() and adds explicit lifetime annotation where needed. Bug: 737010 Change-Id: I242156184a246f572b98bec156206222396ec40e Reviewed-on: https://chromium-review.googlesource.com/558310 Reviewed-by: Ken Rockot <rockot@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#484392} [modify] https://crrev.com/9805c860a67407267ff9c3517c29aacf2238a014/ipc/ipc_sync_channel_unittest.cc
,
Jul 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f41b9335118b7d38f7a35fbec2e29bfc0118f44c commit f41b9335118b7d38f7a35fbec2e29bfc0118f44c Author: tzik <tzik@chromium.org> Date: Mon Jul 10 09:13:33 2017 Ban const-qualified raw pointers to ref-counted objects on base::Bind parameter base::Bind is intended to reject raw pointers to ref-counted objects, however, it fails to reject CVR qualified ones. This CL fixes the check for const-qualified cases. Bug: 737010 Change-Id: I0a3c51d36a240257c66682a2fe061dc514e40835 Reviewed-on: https://chromium-review.googlesource.com/558594 Reviewed-by: Olga Sharonova <olka@chromium.org> Reviewed-by: danakj <danakj@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#485212} [modify] https://crrev.com/f41b9335118b7d38f7a35fbec2e29bfc0118f44c/base/bind_unittest.nc [modify] https://crrev.com/f41b9335118b7d38f7a35fbec2e29bfc0118f44c/base/memory/raw_scoped_refptr_mismatch_checker.h [modify] https://crrev.com/f41b9335118b7d38f7a35fbec2e29bfc0118f44c/chrome/browser/ui/views/frame/browser_window_property_manager_browsertest_win.cc
,
Jul 11 2017
Reopening. The static_assert() in base::Bind has been broken for a while in 4 ways. It failed to reject a raw pointer to a ref-counted type if either: 1. The passed pointer was lvalue. 2. The pointee type was CVR-qualified. 3. The pointee was an incomplete type. 4. Or, external ref-counted types, such as COM, Skia or Blink. (1), (2) are fixed as #c9 and #c14, but (3) and (4) are not yet. |
||||
►
Sign in to add a comment |
||||
Comment 1 by tzik@chromium.org
, Jun 27 2017