Page MenuHomePhabricator

Short circuit breaks with multiple operations
Closed, ResolvedPublic

Description

Recently, I raised the logging for AbuseFilter parser errors to "WARNING", and that caused a long list of errors appear on Logstash. For group0 and group1 alone, roughly 60000 filters failing every hour. Adding the IDs of the faulty filters as well, I could finally determine a sample syntax which causes these errors:

1 = 0 & 1 < 3 * 24 * 3600

Note that the first condition evaluates to false and short-circuit evaluation starts. This should just be evaluated as false, but instead it fails with:

Exception caught: Unexpected "T_OP" at character 18
AFPUserVisibleException at W:\xampp\htdocs\pedia\extensions\AbuseFilter\includes\parser\AbuseFilterParser.php(203)
#0 W:\xampp\htdocs\pedia\extensions\AbuseFilter\includes\parser\AbuseFilterParser.php(186): AbuseFilterParser->doLevelEntry(AFPData)
#1 W:\xampp\htdocs\pedia\extensions\AbuseFilter\includes\parser\AbuseFilterParser.php(172): AbuseFilterParser->intEval(string)
#2 W:\xampp\htdocs\pedia\extensions\AbuseFilter\includes\AbuseFilter.php(499): AbuseFilterParser->evaluateExpression(string)
#3 W:\xampp\htdocs\pedia\extensions\AbuseFilter\includes\api\ApiAbuseFilterEvalExpression.php(10): AbuseFilter::evaluateExpression(string)
#4 W:\xampp\htdocs\pedia\includes\api\ApiMain.php(1596): ApiAbuseFilterEvalExpression->execute()
#5 W:\xampp\htdocs\pedia\includes\api\ApiMain.php(531): ApiMain->executeAction()
#6 W:\xampp\htdocs\pedia\includes\api\ApiMain.php(502): ApiMain->executeActionWithErrorHandling()
#7 W:\xampp\htdocs\pedia\api.php(87): ApiMain->execute()
#8 {main}

This seems to be the minimum non-working example, and it needs first a false condition, then at least two operations.
My first guess is that short-circuit truncates the evaluation too early, so that this is parsed as

(1 = 0 & 1 < 3 * 24) * 3600

and thus fails.

Most important, since this only happen during short-circuit, the syntax check does NOT report anything wrong. This is something which should be fixed too, and I'll open a separate task for it.

Event Timeline

Daimona created this task.Jan 24 2019, 9:40 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 24 2019, 9:40 PM
Daimona triaged this task as High priority.Jan 24 2019, 10:18 PM

Found the cause. It is more or less what I said above. The parser enters in short-circuit with the first condition, and gets to the "<". After that, it finds the 3 and gets inside doLevelMulRels since the current token is now "*". Then it parses the 24, but since shortCircuit is true it breaks out of the loop, here. Thus, the further "*" isn't processed, and the parser just goes up the call stack, up to doLevelEntry. Now the next token is still a "*", and not the special AFPToken::TNONE, and the exception is thrown.

Change 486393 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Fix shortcircuit for consecutive operations

https://gerrit.wikimedia.org/r/486393

Daimona claimed this task.Jan 24 2019, 10:35 PM

Change 486393 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Fix shortcircuit for consecutive operations

https://gerrit.wikimedia.org/r/486393

Daimona closed this task as Resolved.Mar 17 2019, 10:04 AM
Daimona removed a project: Patch-For-Review.