From 53478caee862624fc6d73516f8d64253854b146f Mon Sep 17 00:00:00 2001 From: Piotr Tworek 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 Commit-Queue: Piotr Tworek 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_;