Page MenuHomePhabricator

AbuseFilter parsers throws "Return value of AbuseFilterCachingParser::evalTree() must be an instance of AFPData"
Closed, ResolvedPublic

Description

Error
Error 1
PHP Notice: Undefined offset: -1

[{exception_id}] {exception_url}   ErrorException from line 191 of /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/parser/AbuseFilterCachingParser.php: PHP Notice: Undefined offset: -1

#0 /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/parser/AbuseFilterCachingParser.php(191): MWExceptionHandler::handleError(integer, string, string, integer, array)
#1 /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/parser/AbuseFilterCachingParser.php(101): AbuseFilterCachingParser->evalNode(AFPTreeNode)
#2 /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/parser/AbuseFilterCachingParser.php(60): AbuseFilterCachingParser->evalTree(AFPSyntaxTree)
#3 /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php(291): AbuseFilterCachingParser->intEval(string)
#4 /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/AbuseFilter.php(428): AbuseFilterParser->checkSyntax(string)
#5 /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/Views/AbuseFilterViewTestBatch.php(154): AbuseFilter::checkSyntax(string)
#6 /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/Views/AbuseFilterViewTestBatch.php(144): AbuseFilterViewTestBatch->doTest()
#7 /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/special/SpecialAbuseFilter.php(139): AbuseFilterViewTestBatch->show()
#8 /srv/mediawiki/php-1.35.0-wmf.4/includes/specialpage/SpecialPage.php(575): SpecialAbuseFilter->execute(string)
#9 /srv/mediawiki/php-1.35.0-wmf.4/includes/specialpage/SpecialPageFactory.php(607): SpecialPage->run(string)
#10 /srv/mediawiki/php-1.35.0-wmf.4/includes/MediaWiki.php(298): MediaWiki\Special\SpecialPageFactory->executePath(Title, RequestContext)
#11 /srv/mediawiki/php-1.35.0-wmf.4/includes/MediaWiki.php(967): MediaWiki->performRequest()
#12 /srv/mediawiki/php-1.35.0-wmf.4/includes/MediaWiki.php(530): MediaWiki->main()
#13 /srv/mediawiki/php-1.35.0-wmf.4/index.php(44): MediaWiki->run()
#14 /srv/mediawiki/w/index.php(3): require(string)
#15 {main}
Error 2
TypeError: Return value of AbuseFilterCachingParser::evalTree() must be an instance of AFPData, null returned

#0 /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/parser/AbuseFilterCachingParser.php(60): AbuseFilterCachingParser->evalTree(AFPSyntaxTree)
#1 /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php(291): AbuseFilterCachingParser->intEval(string)
#2 /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/AbuseFilter.php(428): AbuseFilterParser->checkSyntax(string)
#3 /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/Views/AbuseFilterViewTestBatch.php(154): AbuseFilter::checkSyntax(string)
#4 /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/Views/AbuseFilterViewTestBatch.php(144): AbuseFilterViewTestBatch->doTest()
#5 /srv/mediawiki/php-1.35.0-wmf.4/extensions/AbuseFilter/includes/special/SpecialAbuseFilter.php(139): AbuseFilterViewTestBatch->show()
#6 /srv/mediawiki/php-1.35.0-wmf.4/includes/specialpage/SpecialPage.php(575): SpecialAbuseFilter->execute(string)
#7 /srv/mediawiki/php-1.35.0-wmf.4/includes/specialpage/SpecialPageFactory.php(607): SpecialPage->run(string)
#8 /srv/mediawiki/php-1.35.0-wmf.4/includes/MediaWiki.php(298): MediaWiki\Special\SpecialPageFactory->executePath(Title, RequestContext)
#9 /srv/mediawiki/php-1.35.0-wmf.4/includes/MediaWiki.php(967): MediaWiki->performRequest()
#10 /srv/mediawiki/php-1.35.0-wmf.4/includes/MediaWiki.php(530): MediaWiki->main()
#11 /srv/mediawiki/php-1.35.0-wmf.4/index.php(44): MediaWiki->run()
#12 /srv/mediawiki/w/index.php(3): require(string)
#13 {main}
Steps to Reproduce:

Run the following on Special:AbuseFilter/test:

Input
[0][-1]

Actual Results:

Internal Error
[Xb@w-ApAIC4AAEPpiiUAAABD] 2019-11-04 05:02:52: การยกเว้นข้อผิดพลาดร้ายแรงของประเภท "TypeError"

(from thwiki)

Expected Results:

A proper error

Details

Request ID
Xb@w-ApAIC4AAEPpiiUAAABD
Related Gerrit Patches:
mediawiki/extensions/AbuseFilter : masterExplicitly forbid negative indexes in arrays

Event Timeline

Nullzero created this task.Nov 4 2019, 5:03 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 4 2019, 5:03 AM
Aklapper renamed this task from Array indexing needs lower bound check to Array indexing needs lower bound check ("PHP Notice: Undefined offset: -1").Nov 4 2019, 8:43 AM
Aklapper changed the subtype of this task from "Bug Report" to "Production Error".
Aklapper set Request ID to Xb@w-ApAIC4AAEPpiiUAAABD.
Aklapper edited Stack Trace. (Show Details)
Daimona added a subscriber: Daimona.Nov 4 2019, 2:33 PM

Weird things happen.

x := ['foo'];

x[-1]

->fails hard

x := ['foo'];

x[-1] := 2;

x[-1]

-> no error

More generally, AF doesn't support array "with holes". Hence, the following is also invalid:

x := ['foo'];

x[5] := 2;

although this fails with a different error (outofbounds).

This happens because the "check for holes" is:

if ( count( current_array_object ) <= requested_index ) {
  throw
}

Depending on how we want to fix this, there are various ways forward. One is to add || requested_index < 0. Another one is to allow negative indexes, or even allow array with holes.

@Nullzero what would you suggest?

To avoid surprising users, I think we should mandate that index must be between 0 and |array| - 1.

To avoid surprising users, I think we should mandate that index must be between 0 and |array| - 1.

Yeah, makes sense. We can also allow negative index in the future, if we want to.

Change 548324 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Forbid negative indexes in arrays

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

Daimona claimed this task.Nov 4 2019, 5:48 PM
Krinkle renamed this task from Array indexing needs lower bound check ("PHP Notice: Undefined offset: -1") to AbuseFilter parsers throws "Return value of AbuseFilterCachingParser::evalTree() must be an instance of AFPData".Nov 7 2019, 9:37 PM
Krinkle updated the task description. (Show Details)
Krinkle edited Stack Trace. (Show Details)
Krinkle closed this task as Resolved.Nov 8 2019, 5:56 AM

Change 548324 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Explicitly forbid negative indexes in arrays

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