New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 618529 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug

Blocking:
issue 616322



Sign in to add a comment

HostContentSettingsMap::SetWebsiteSettingDefaultScope leaks its base::Value argument if either its primary or secondary URL form invalid patterns

Project Member Reported by dominickn@chromium.org, Jun 9 2016

Issue description

HostContentSettingsMap::SetWebsiteSettingDefaultScope takes a raw base::Value* pointer, and its clients typically release() a std::unique_ptr when calling it. However, this method returns without doing anything if provided with an invalid primary or secondary URL pattern. This leaks the value.

This method should be changed to take a std::unique_ptr instead.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7e7fc45f0d5f84430707280a18dfe7d6217aed8

commit d7e7fc45f0d5f84430707280a18dfe7d6217aed8
Author: dominickn <dominickn@chromium.org>
Date: Thu Jun 09 07:30:33 2016

Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope
takes a raw base::Value*, and its clients typically release() a
std::unique_ptr<base::Value> when calling it. However, this method
returns without doing anything if provided with an invalid primary or
secondary URL pattern (e.g. the view source page). When this happens,
the base::Value is leaked, as it has been released() by its previous
owner without being taken into a unique_ptr in
SetWebsiteSettingsDefaultScope.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to use either std::move or base::WrapUnique.

BUG= 618529 
TBR=felt@chromium.org,mkwst@chromium.org,benwells@chromium.org

Review-Url: https://codereview.chromium.org/2054623002
Cr-Commit-Position: refs/heads/master@{#398806}

[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/chrome/browser/banners/app_banner_settings_helper.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/chrome/browser/browsing_data/browsing_data_remover_unittest.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/chrome/browser/content_settings/host_content_settings_map_unittest.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/chrome/browser/engagement/site_engagement_score.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/chrome/browser/permissions/chooser_context_base.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/chrome/browser/ssl/ssl_browser_tests.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/components/content_settings/core/browser/host_content_settings_map.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/components/content_settings/core/browser/host_content_settings_map.h

Status: Fixed (was: Started)
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7e7fc45f0d5f84430707280a18dfe7d6217aed8

commit d7e7fc45f0d5f84430707280a18dfe7d6217aed8
Author: dominickn <dominickn@chromium.org>
Date: Thu Jun 09 07:30:33 2016

Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope
takes a raw base::Value*, and its clients typically release() a
std::unique_ptr<base::Value> when calling it. However, this method
returns without doing anything if provided with an invalid primary or
secondary URL pattern (e.g. the view source page). When this happens,
the base::Value is leaked, as it has been released() by its previous
owner without being taken into a unique_ptr in
SetWebsiteSettingsDefaultScope.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to use either std::move or base::WrapUnique.

BUG= 618529 
TBR=felt@chromium.org,mkwst@chromium.org,benwells@chromium.org

Review-Url: https://codereview.chromium.org/2054623002
Cr-Commit-Position: refs/heads/master@{#398806}

[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/chrome/browser/banners/app_banner_settings_helper.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/chrome/browser/browsing_data/browsing_data_remover_unittest.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/chrome/browser/content_settings/host_content_settings_map_unittest.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/chrome/browser/engagement/site_engagement_score.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/chrome/browser/permissions/chooser_context_base.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/chrome/browser/ssl/chrome_ssl_host_state_delegate.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/chrome/browser/ssl/ssl_browser_tests.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/components/content_settings/core/browser/host_content_settings_map.cc
[modify] https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8/components/content_settings/core/browser/host_content_settings_map.h

Sign in to add a comment