Page MenuHomePhabricator
Create Task
Maniphest T237219

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

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
SubjectRepoBranchLines +/-
Explicitly forbid negative indexes in arraysmediawiki/extensions/AbuseFiltermaster+39 -0
Customize query in gerrit

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)
Aklapper added a project: Wikimedia-production-error.
Daimona subscribed.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?

Nullzero added a comment.Nov 4 2019, 5:06 PM

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

Daimona added a comment.Nov 4 2019, 5:37 PM
In T237219#5632317, @Nullzero wrote:

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.

gerritbot added a comment.Nov 4 2019, 5:47 PM

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

gerritbot added a project: Patch-For-Review.Nov 4 2019, 5:47 PM
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 moved this task from Untriaged to Nov2019/1.35.wmf.5+ on the Wikimedia-production-error board.
Krinkle closed this task as Resolved.Nov 8 2019, 5:56 AM
gerritbot added a comment.Nov 8 2019, 6:05 AM

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

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

Maintenance_bot removed a project: Patch-For-Review.Nov 8 2019, 6:10 AM
ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.8; 2019-11-26).Nov 8 2019, 7:00 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL