Fix domain matching logic #162
Conversation
This also makes it more performant for execution time.
| } | ||
| for (int i = 0; i < numNoFingerprintAntiDomainOnlyExceptionFilters; i++) { | ||
| newNoFingerprintAntiDomainOnlyExceptionFilters[i].swapData(&(noFingerprintAntiDomainOnlyExceptionFilters[i])); | ||
| } |
There was a problem hiding this comment.
Using the old way of simply memcpy over here no longer works because Filter's store pointers now, and they manage their own memory. So when a Filter was destroyed, it would free the pointers twice.
There was a problem hiding this comment.
The perf here doesn't matter btw because this is only for parsing logic. We use deserialized dat files in production.
bbondy
force-pushed
the
domain-matching-hashset
branch
2 times, most recently
from
c4ac3f7 to
3c7bc90
Compare
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
There was a problem hiding this comment.
This class is used in HashSet's which store the domains and antiDomains for each Filter.
| } | ||
| if (data) { | ||
| delete[] data; | ||
| if (domains) { |
There was a problem hiding this comment.
domains and antiDomains are lazily created, so check if we need to delete them.
| } | ||
| if (host) { | ||
| delete[] host; | ||
| if (!borrowed_data) { |
There was a problem hiding this comment.
This is just a refactoring of an early return to not have an early return. Code in this block is the same as before.
bbondy
force-pushed
the
domain-matching-hashset
branch
from
3c7bc90 to
c7f2bfa
Compare
| const char *p = input; | ||
| if (anti) { | ||
| if (len >= 1 && p[0] != '~') { | ||
| bool Filter::containsDomain(const char* domain, size_t domainLen, |
There was a problem hiding this comment.
See the header file for a description of this. Mainly it just checks if the specified domain is inside the filter's list of domains (or anti domains).
| @@ -298,7 +270,8 @@ void Filter::parseOption(const char *input, int len) { | |||
| *pFilterOption = static_cast<FilterOption>(*pFilterOption | FOThirdParty); | |||
| } else if (!strncmp(pStart, "first-party", len)) { | |||
| // Same as ~third-party | |||
| *pFilterOption = static_cast<FilterOption>(*pFilterOption | FONotThirdParty); | |||
| *pFilterOption = static_cast<FilterOption>( | |||
| *pFilterOption | FONotThirdParty); | |||
| @@ -387,6 +360,44 @@ bool Filter::hasUnsupportedOptions() const { | |||
| return (filterOption & FOUnsupportedSoSkipCheck) != 0; | |||
| } | |||
|
|
|||
| bool Filter::contextDomainMatchesFilter(const char *contextDomain) { | |||
There was a problem hiding this comment.
This checks if a context domain is applicable to this filter.
For example if foo.example.com is the context domain then it will check if this filter is applicable to both foo.example.com and example.com.
Domain matching logic was complex and didn't work in all cases. This caused us to miss an exception allow rule from: brave/brave-browser#2843
This rewrites the domain matching logic to be much faster on checks, but use a bit more memory.
Note that domain matching code is well tested already before this PR, so there's a lot of tests which ensure the safety of this change. New tests were also added to cover the new cases.