Skip to content

Commit

Permalink
[M110] Fix buggy use of memset in Rankings::Iterator
Browse files Browse the repository at this point in the history
Using memset over this causes the raw_ptr's private ptr to be reset
incorrectly, without updating the refcount. It causes the quarantine size to increase infinitely.

This patch is likely to be merged into beta, so I made the minimal
change. I would like to refactor this more in a second step.

Plan:
1. Minimal fix (this patch)
2. Refactor: https://chromium-review.googlesource.com/c/chromium/src/+/4189439

(cherry picked from commit 70e0aee)

Bug: 1409814
Change-Id: I0f7f057fe04ac5e90e9f720473c03521752e0db7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4188566
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1096455}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4200666
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/branch-heads/5481@{#722}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
ArthurSonzogni authored and Chromium LUCI CQ committed Jan 27, 2023
1 parent 1f33949 commit 5efa6b7
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 7 deletions.
8 changes: 4 additions & 4 deletions net/disk_cache/blockfile/rankings.cc
Original file line number Diff line number Diff line change
Expand Up @@ -206,17 +206,17 @@ Rankings::ScopedRankingsBlock::ScopedRankingsBlock(Rankings* rankings,
CacheRankingsBlock* node)
: std::unique_ptr<CacheRankingsBlock>(node), rankings_(rankings) {}

Rankings::Iterator::Iterator() {
memset(this, 0, sizeof(Iterator));
}
Rankings::Iterator::Iterator() = default;

void Rankings::Iterator::Reset() {
if (my_rankings) {
for (auto* node : nodes) {
ScopedRankingsBlock(my_rankings, node);
}
}
memset(this, 0, sizeof(Iterator));
my_rankings = nullptr;
nodes = {nullptr, nullptr, nullptr};
list = List::NO_USE;
}

Rankings::Rankings() = default;
Expand Down
11 changes: 8 additions & 3 deletions net/disk_cache/blockfile/rankings.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <list>
#include <memory>

#include <array>
#include "base/memory/raw_ptr.h"
#include "net/disk_cache/blockfile/addr.h"
#include "net/disk_cache/blockfile/mapped_file.h"
Expand Down Expand Up @@ -96,13 +97,17 @@ class Rankings {

// If we have multiple lists, we have to iterate through all at the same time.
// This structure keeps track of where we are on the iteration.
// TODO(https://crbug.com/1409814) refactor this struct to make it clearer
// this owns the `nodes`.
struct Iterator {
Iterator();
void Reset();

List list; // Which entry was returned to the user.
CacheRankingsBlock* nodes[3]; // Nodes on the first three lists.
raw_ptr<Rankings> my_rankings;
// Which entry was returned to the user.
List list = List::NO_USE;
// Nodes on the first three lists.
std::array<CacheRankingsBlock*, 3> nodes = {nullptr, nullptr, nullptr};
raw_ptr<Rankings> my_rankings = nullptr;
};

Rankings();
Expand Down

0 comments on commit 5efa6b7

Please sign in to comment.