Skip to content

Commit

Permalink
Fix regression of isBlockImportant()
Browse files Browse the repository at this point in the history
isBlockImportant() was relying strictly on the hash bits
to detect whether a matching filter was `important`, but
this approach regressed with changes with how `important`
filters are compiled. This commit fixed this by no longer
relying on the hash bits but rather on an internal
register variable being set by `important` filters when
they match.

I couldn't find any actual cases in default filter lists
(including a couple of default regional lists) that the
regression is having any effect, due to the limited cases
for which isBlockImportant() is called.

A test was added in a previous commit to detect such
regression in the future:
- a76935b
  • Loading branch information
gorhill committed Oct 6, 2021
1 parent 6464002 commit c0cba22
Showing 1 changed file with 14 additions and 6 deletions.
20 changes: 14 additions & 6 deletions src/js/static-net-filtering.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ let $docDomain = '';
let $tokenBeg = 0;
let $patternMatchLeft = 0;
let $patternMatchRight = 0;
let $isBlockImportant = false;

const $docEntity = {
entity: undefined,
Expand Down Expand Up @@ -298,9 +299,6 @@ class LogData {
isRegex: false,
};
filterUnits[iunit].logData(logData);
if ( (categoryBits & Important) !== 0 ) {
logData.options.unshift('important');
}
if ( (categoryBits & ThirdParty) !== 0 ) {
logData.options.unshift('3p');
} else if ( (categoryBits & FirstParty) !== 0 ) {
Expand Down Expand Up @@ -605,7 +603,7 @@ registerFilterClass(FilterTrue);

const FilterImportant = class {
match() {
return true;
return ($isBlockImportant = true);
}

logData(details) {
Expand Down Expand Up @@ -3522,6 +3520,14 @@ class FilterCompiler {
this.action |= HEADERS;
}

// Important
//
// IMPORTANT: must always appear at the end of the sequence, so as to
// ensure $isBlockImportant is set only for matching filters.
if ( (this.optionUnitBits & this.IMPORTANT_BIT) !== 0 ) {
units.push(FilterImportant.compile());
}

// Modifier
//
// IMPORTANT: the modifier unit MUST always appear first in a sequence
Expand All @@ -3545,7 +3551,6 @@ class FilterCompiler {
// meant to override.
if ( (this.action & ActionBitsMask) === BlockImportant ) {
this.action &= ~Important;
units.push(FilterImportant.compile());
this.compileToAtomicFilter(
FilterCompositeAll.compile(units),
writer
Expand Down Expand Up @@ -4259,6 +4264,7 @@ FilterContainer.prototype.matchRequestReverse = function(type, url) {
$requestURL = urlTokenizer.setURL(url);
$requestURLRaw = url;
$requestTypeValue = (typeBits & TypeBitsMask) >>> TypeBitsOffset;
$isBlockImportant = false;
this.$filterUnit = 0;

// These registers will be used by various filters
Expand Down Expand Up @@ -4327,11 +4333,13 @@ FilterContainer.prototype.matchRequest = function(fctxt, modifiers = 0) {
$docEntity.reset();
$requestHostname = fctxt.getHostname();
$requestTypeValue = (typeBits & TypeBitsMask) >>> TypeBitsOffset;
$isBlockImportant = false;

// Evaluate block realm before allow realm, and allow realm before
// block-important realm, i.e. by order of likelihood of a match.
const r = this.realmMatchString(BlockAction, typeBits, partyBits);
if ( r || (modifiers & 0b0010) !== 0 ) {
if ( $isBlockImportant ) { return 1; }
if ( this.realmMatchString(AllowAction, typeBits, partyBits) ) {
if ( this.realmMatchString(BlockImportant, typeBits, partyBits) ) {
return 1;
Expand Down Expand Up @@ -4538,7 +4546,7 @@ FilterContainer.prototype.toLogData = function() {
/******************************************************************************/

FilterContainer.prototype.isBlockImportant = function() {
return (this.$catBits & ActionBitsMask) === BlockImportant;
return this.$filterUnit !== 0 && $isBlockImportant;
};

/******************************************************************************/
Expand Down

0 comments on commit c0cba22

Please sign in to comment.