File fix-invalid-end-iterator-usage-in-CookieMonster... of Package chromium.14883

Overview Repositories Revisions Requests Users Attributes Meta

File fix-invalid-end-iterator-usage-in-CookieMonster.patch of Package chromium.14883

From 53478caee862624fc6d73516f8d64253854b146f Mon Sep 17 00:00:00 2001
From: Piotr Tworek <ptworek@vewd.com>
Date: Mon, 31 Aug 2020 21:03:58 +0000
Subject: [PATCH] Fix invalid "end" iterator usage in CookieMonster.

Commit 229623d76e8baf714c8569c9f4efc5de266cef8b has introduced the following
code in cookie_monster.cc.

// If this is the first cookie in |cookies_| with this key, increment the
// |num_keys_| counter.
bool different_prev =
    inserted == cookies_.begin() || std::prev(inserted)->first != key;
bool different_next =
    inserted == cookies_.end() || std::next(inserted)->first != key;
if (different_prev && different_next)
  ++num_keys_;

The "inserted" iterator is something that has been returned from
std::multimap::insert. At first glance it looks reasonable. The code
tries to determine if there are already similar elements with the same
key in the map. Unfortunately the expression calculating the value of
different_next can potentially use the end iterator to the map. The
"inserted == cookies_.end()" part of the expression will always evaluate
to false since the newly inserted element has to be in the map and
cookies_.end() points to the first element outside the map. If the
inserted happens to be the last element in the map the second part of
the expression will grab the end iterator by calling std::next(inserted)
and then will try to use it leading to invalid memory access.

Given the fact that cookies_ is a std::multimap we should not even need
to calculate the value of different_next. It should always be true.

"If the container has elements with equivalent key, inserts at the
  upper bound of that range.(since C++11)"

See: https://en.cppreference.com/w/cpp/container/multimap/insert

Bug: 1120240
Change-Id: I8928c294ac4daf72349a2331b31b017c1d015da0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2368872
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Commit-Queue: Piotr Tworek <ptworek@vewd.com>
Cr-Commit-Position: refs/heads/master@{#803260}
---
 net/cookies/cookie_monster.cc | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/cookies/cookie_monster.cc b/net/cookies/cookie_monster.cc
index 265deed0e52..140b61a81dc 100644
--- a/net/cookies/cookie_monster.cc
+++ b/net/cookies/cookie_monster.cc
@@ -1151,9 +1151,14 @@ CookieMonster::CookieMap::iterator CookieMonster::InternalInsertCookie(
   // |num_keys_| counter.
   bool different_prev =
       inserted == cookies_.begin() || std::prev(inserted)->first != key;
-  bool different_next =
-      inserted == cookies_.end() || std::next(inserted)->first != key;
-  if (different_prev && different_next)
+  // According to std::multiqueue documentation:
+  // "If the container has elements with equivalent key, inserts at the upper
+  // bound of that range. (since C++11)"
+  // This means that "inserted" iterator either points to the last element in
+  // the map, or the element succeeding it has to have different key.
+  DCHECK(std::next(inserted) == cookies_.end() ||
+         std::next(inserted)->first != key);
+  if (different_prev)
     ++num_keys_;
 
   return inserted;
@@ -1381,7 +1386,7 @@ void CookieMonster::InternalDeleteCookie(CookieMap::iterator it,
   bool different_prev =
       it == cookies_.begin() || std::prev(it)->first != it->first;
   bool different_next =
-      it == cookies_.end() || std::next(it)->first != it->first;
+      std::next(it) == cookies_.end() || std::next(it)->first != it->first;
   if (different_prev && different_next)
     --num_keys_;

Places

File fix-invalid-end-iterator-usage-in-CookieMonster.patch of Package chromium.14883

Places