Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFix domain matching logic #162
Conversation
| } | ||
| for (int i = 0; i < numNoFingerprintAntiDomainOnlyExceptionFilters; i++) { | ||
| newNoFingerprintAntiDomainOnlyExceptionFilters[i].swapData(&(noFingerprintAntiDomainOnlyExceptionFilters[i])); | ||
| } |
This comment has been minimized.
This comment has been minimized.
bbondy
Jan 23, 2019
Author
Member
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.
This comment has been minimized.
This comment has been minimized.
bbondy
Jan 23, 2019
Author
Member
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
| * 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/. */ | ||
|
|
This comment has been minimized.
This comment has been minimized.
bbondy
Jan 23, 2019
Author
Member
This class is used in HashSet's which store the domains and antiDomains for each Filter.
| } | ||
| if (data) { | ||
| delete[] data; | ||
| if (domains) { |
This comment has been minimized.
This comment has been minimized.
bbondy
Jan 23, 2019
Author
Member
domains and antiDomains are lazily created, so check if we need to delete them.
| } | ||
| if (host) { | ||
| delete[] host; | ||
| if (!borrowed_data) { |
This comment has been minimized.
This comment has been minimized.
bbondy
Jan 23, 2019
Author
Member
This is just a refactoring of an early return to not have an early return. Code in this block is the same as before.
| const char *p = input; | ||
| if (anti) { | ||
| if (len >= 1 && p[0] != '~') { | ||
| bool Filter::containsDomain(const char* domain, size_t domainLen, |
This comment has been minimized.
This comment has been minimized.
bbondy
Jan 23, 2019
Author
Member
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); | |||
This comment has been minimized.
This comment has been minimized.
| @@ -387,6 +360,44 @@ bool Filter::hasUnsupportedOptions() const { | |||
| return (filterOption & FOUnsupportedSoSkipCheck) != 0; | |||
| } | |||
|
|
|||
| bool Filter::contextDomainMatchesFilter(const char *contextDomain) { | |||
This comment has been minimized.
This comment has been minimized.
bbondy
Jan 23, 2019
Author
Member
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.
|
nothing major that caught my eyes. but needs to tested well before production. Hopefully we will see if something wrong on dev builds. |
bbondy commentedJan 23, 2019
•
edited
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.