Skip to content
This repository was archived by the owner on Apr 10, 2025. It is now read-only.

Commit 0654743

Browse files
Jan-Willem Maessencrowell
authored andcommitted
Fix FastWildcardGroup bug. We insert all-wildcard patterns in *reverse* order,
and should match them forwards as a result. Add a couple of tests beyond Steve's initial repro. #1294
1 parent 921fd71 commit 0654743

File tree

3 files changed

+63
-13
lines changed

3 files changed

+63
-13
lines changed

pagespeed/kernel/base/fast_wildcard_group.cc

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,6 @@
3333
namespace net_instaweb {
3434

3535
namespace {
36-
// Don't generate a hash unless there are this many
37-
// non-wildcard-only patterns.
38-
const int kMinPatterns = 11;
39-
4036
// Maximum rolling hash window size
4137
const int32 kMaxRollingHashWindow = 256;
4238

@@ -266,23 +262,24 @@ bool FastWildcardGroup::Match(const StringPiece& str, bool allow) const {
266262
return allow;
267263
}
268264
int max_effective_index = kNoEntry;
269-
// Start by matching against all-wildcard patterns in reverse order,
270-
// stopping if a match is found (since earlier matches will have
271-
// a smaller index and be overridden by the already-found match).
272-
// TODO(jmaessen): These patterns all devolve to
273-
// a string length check (== or >=). Consider optimizing them.
274-
for (int i = wildcard_only_indices_.size() - 1; i >= 0; --i) {
265+
// Start by matching against all-wildcard patterns in reverse order. Their
266+
// indices are stored in reverse index order in wildcard_only_indices, so
267+
// traverse it forwards. Stop if a match is found (since earlier matches will
268+
// have a smaller index and be overridden by the already-found match).
269+
// TODO(jmaessen): These patterns all devolve to a string length check (== or
270+
// >=). Consider optimizing them.
271+
for (int i = 0, sz = wildcard_only_indices_.size(); i < sz; ++i) {
275272
int index = wildcard_only_indices_[i];
276273
if (wildcards_[index]->Match(str)) {
277274
max_effective_index = effective_indices_[index];
278275
break;
279276
}
280277
}
278+
int exit_effective_index = wildcards_.size() - 1;
281279
int rolling_end = str.size() - rolling_hash_length;
282-
if (rolling_end >= 0) {
280+
if (max_effective_index < exit_effective_index && rolling_end >= 0) {
283281
// Do a Rabin-Karp rolling match through the string.
284282
uint64 rolling_hash = RollingHash(str.data(), 0, rolling_hash_length);
285-
int exit_effective_index = wildcards_.size() - 1;
286283
// Uses signed arithmetic for correct comparison below.
287284
for (int ofs = 0;
288285
max_effective_index < exit_effective_index && ofs <= rolling_end; ) {

pagespeed/kernel/base/fast_wildcard_group.h

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,11 @@ don't get long, and all failed probes terminate in an empty bucket.
9393

9494
class FastWildcardGroup {
9595
public:
96+
// Don't generate a hash unless there are this many non-wildcard-only
97+
// patterns. Exposed for testing purposes (we can't use FRIEND_TEST here for
98+
// open-source dependency reasons).
99+
static const int kMinPatterns = 11;
100+
96101
FastWildcardGroup()
97102
: rolling_hash_length_(kUncompiled) { }
98103
FastWildcardGroup(const FastWildcardGroup& src)
@@ -150,7 +155,7 @@ class FastWildcardGroup {
150155
// Information that is computed during compilation.
151156
mutable std::vector<uint64> rolling_hashes_; // One per wildcard
152157
mutable std::vector<int> effective_indices_; // One per wildcard
153-
mutable std::vector<int> wildcard_only_indices_;
158+
mutable std::vector<int> wildcard_only_indices_; // Reverse order
154159
mutable std::vector<int> pattern_hash_index_; // hash table
155160
mutable AtomicInt32 rolling_hash_length_;
156161

pagespeed/kernel/base/fast_wildcard_group_test.cc

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,54 @@ TEST_F(FastWildcardGroupTest, AppendSequenceLarge) {
139139
Append();
140140
}
141141

142+
TEST_F(FastWildcardGroupTest, AllowDisallowCompiled) {
143+
FastWildcardGroup group;
144+
group.Disallow("*");
145+
// Pad the group with irrelevant stuff to force it to be compiled.
146+
for (int i = 0; i < FastWildcardGroup::kMinPatterns; ++i) {
147+
group.Allow("a");
148+
}
149+
group.Allow("*");
150+
151+
EXPECT_TRUE(group.Match("a", true));
152+
EXPECT_TRUE(group.Match("b", true));
153+
EXPECT_TRUE(group.Match("c", true));
154+
}
155+
156+
TEST_F(FastWildcardGroupTest, AllowDisallowCompiledLarger) {
157+
FastWildcardGroup group;
158+
group.Allow("*");
159+
// Pad the group with irrelevant stuff to force it to be compiled.
160+
for (int i = 0; i < FastWildcardGroup::kMinPatterns; ++i) {
161+
group.Disallow("a");
162+
}
163+
group.Disallow("*");
164+
for (int i = 0; i < FastWildcardGroup::kMinPatterns; ++i) {
165+
group.Allow("c");
166+
}
167+
168+
EXPECT_FALSE(group.Match("a", true));
169+
EXPECT_FALSE(group.Match("b", true));
170+
EXPECT_TRUE(group.Match("c", true));
171+
}
172+
173+
TEST_F(FastWildcardGroupTest, AllowDisallowLargeWildcardOnly) {
174+
FastWildcardGroup group;
175+
group.Allow("?");
176+
// Pad the group with irrelevant stuff to force it to be compiled.
177+
for (int i = 0; i < FastWildcardGroup::kMinPatterns; ++i) {
178+
group.Allow("aa");
179+
}
180+
group.Disallow("??");
181+
182+
EXPECT_TRUE(group.Match("a", true));
183+
EXPECT_FALSE(group.Match("aa", true));
184+
EXPECT_TRUE(group.Match("aaa", true));
185+
EXPECT_TRUE(group.Match("a", false));
186+
EXPECT_FALSE(group.Match("aa", false));
187+
EXPECT_FALSE(group.Match("aaa", false));
188+
}
189+
142190
TEST_F(FastWildcardGroupTest, HardCodedDefault) {
143191
HardCodedDefault();
144192
}

0 commit comments

Comments
 (0)