Page MenuHomePhabricator

Occasional phan-taint-check-plugin crashes with “Illegal offset type in isset or empty”
Closed, ResolvedPublic

Description

I noticed this in build #83250:

Analyzing files...
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░/mediawiki/extensions/Wikibase/vendor/mediawiki/phan-taint-check-plugin/src/MWVisitor.php:404 [2] Illegal offset type in isset or empty
(Phan 2.6.1 crashed when parsing/analyzing 'client/includes/Api/ApiPropsEntityUsage.php')
More details:
#2: MWVisitor->handleGetQueryInfoReturn() called at [/mediawiki/extensions/Wikibase/vendor/mediawiki/phan-taint-check-plugin/src/MWVisitor.php:354] Args: [ast\Node({"kind":129,"flags":3,"lineno":755,"children":[{"kind":525,"flags":0,"lineno":755,"children":{"value":{"kind":513,"flags":0,"lineno":755,"children":{"expr":{"kind":256,"flags":0,"lineno":755,"children":{"name":"this"}},"prop":"tables"}},"key":"tables"}},{"kind":525,"flags":0,"lineno":756,"children":{"value":{"kind":513,"flags":0,"lineno":756,"children":{"expr":{"kind":256,"flags":0,"lineno":756,"children":{"name":"this"}},"prop":"fields"}},"key":"fields"}},{"kind":525,"flags":0,"lineno":757,"children":{"value":{"kind":513,"flags":0,"lineno":757,"children":{"expr":{"kind":256,"flags":0,"lineno":757,"children":{"name":"this"}},"prop":"conds"}},"key":"conds"}},{"kind":525,"flags":0,"lineno":758,"children":{"value":{"kind":513,"flags":0,"lineno":758,"children":{"expr":{"kind":256,"flags":0,"lineno":758,"children":{"name":"this"}},"p...

(The Gerrit change for this build doesn’t touch ApiPropsEntityUsage.php, or indeed any file below client/.) But looking through the mwext-php72-phan-docker build history I also found some other occurrences of what seems to be the same error:

#83256

18:55:00 Analyzing files...
18:55:14 ░░░░░░░░░░░░░░/mediawiki/extensions/AbuseFilter/vendor/mediawiki/phan-taint-check-plugin/src/MWVisitor.php:404 [2] Illegal offset type in isset or empty
18:55:20 (Phan 2.6.1 crashed when parsing/analyzing 'includes/api/ApiQueryAbuseFilters.php')
18:55:20 More details:
18:55:20 #2: MWVisitor->handleGetQueryInfoReturn() called at [/mediawiki/extensions/AbuseFilter/vendor/mediawiki/phan-taint-check-plugin/src/MWVisitor.php:354] Args: [ast\Node({"kind":129,"flags":3,"lineno":755,"children":[{"kind":525,"flags":0,"lineno":755,"children":{"value":{"kind":513,"flags":0,"lineno":755,"children":{"expr":{"kind":256,"flags":0,"lineno":755,"children":{"name":"this"}},"prop":"tables"}},"key":"tables"}},{"kind":525,"flags":0,"lineno":756,"children":{"value":{"kind":513,"flags":0,"lineno":756,"children":{"expr":{"kind":256,"flags":0,"lineno":756,"children":{"name":"this"}},"prop":"fields"}},"key":"fields"}},{"kind":525,"flags":0,"lineno":757,"children":{"value":{"kind":513,"flags":0,"lineno":757,"children":{"expr":{"kind":256,"flags":0,"lineno":757,"children":{"name":"this"}},"prop":"conds"}},"key":"conds"}},{"kind":525,"flags":0,"lineno":758,"children":{"value":{"kind":513,"flags":0,"lineno":758,"children":{"expr":{"kind":256,"flags":0,"lineno":758,"children":{"name":"this"}}...

#83254

18:54:19 Analyzing files...
18:54:34 ░░░░░░░░░░░░░░/mediawiki/extensions/AbuseFilter/vendor/mediawiki/phan-taint-check-plugin/src/MWVisitor.php:404 [2] Illegal offset type in isset or empty
18:54:34 (Phan 2.6.1 crashed when parsing/analyzing 'includes/api/ApiQueryAbuseFilters.php')
18:54:34 More details:
18:54:34 #2: MWVisitor->handleGetQueryInfoReturn() called at [/mediawiki/extensions/AbuseFilter/vendor/mediawiki/phan-taint-check-plugin/src/MWVisitor.php:354] Args: [ast\Node({"kind":129,"flags":3,"lineno":755,"children":[{"kind":525,"flags":0,"lineno":755,"children":{"value":{"kind":513,"flags":0,"lineno":755,"children":{"expr":{"kind":256,"flags":0,"lineno":755,"children":{"name":"this"}},"prop":"tables"}},"key":"tables"}},{"kind":525,"flags":0,"lineno":756,"children":{"value":{"kind":513,"flags":0,"lineno":756,"children":{"expr":{"kind":256,"flags":0,"lineno":756,"children":{"name":"this"}},"prop":"fields"}},"key":"fields"}},{"kind":525,"flags":0,"lineno":757,"children":{"value":{"kind":513,"flags":0,"lineno":757,"children":{"expr":{"kind":256,"flags":0,"lineno":757,"children":{"name":"this"}},"prop":"conds"}},"key":"conds"}},{"kind":525,"flags":0,"lineno":758,"children":{"value":{"kind":513,"flags":0,"lineno":758,"children":{"expr":{"kind":256,"flags":0,"lineno":758,"children":{"name":"this"}}...

Event Timeline

Another one of these in WikiLambda change 641262, #83260:

00:01:25.748 Analyzing files...
00:01:28.454 ░░/mediawiki/extensions/WikiLambda/vendor/mediawiki/phan-taint-check-plugin/src/MWVisitor.php:404 [2] Illegal offset type in isset or empty
00:01:28.454 (Phan 2.6.1 crashed when parsing/analyzing 'includes/API/ApiQueryZObjectLabels.php')
00:01:28.454 More details:
00:01:28.485 #2: MWVisitor->handleGetQueryInfoReturn() called at [/mediawiki/extensions/WikiLambda/vendor/mediawiki/phan-taint-check-plugin/src/MWVisitor.php:354] Args: [ast\Node({"kind":129,"flags":3,"lineno":755,"children":[{"kind":525,"flags":0,"lineno":755,"children":{"value":{"kind":513,"flags":0,"lineno":755,"children":{"expr":{"kind":256,"flags":0,"lineno":755,"children":{"name":"this"}},"prop":"tables"}},"key":"tables"}},{"kind":525,"flags":0,"lineno":756,"children":{"value":{"kind":513,"flags":0,"lineno":756,"children":{"expr":{"kind":256,"flags":0,"lineno":756,"children":{"name":"this"}},"prop":"fields"}},"key":"fields"}},{"kind":525,"flags":0,"lineno":757,"children":{"value":{"kind":513,"flags":0,"lineno":757,"children":{"expr":{"kind":256,"flags":0,"lineno":757,"children":{"name":"this"}},"prop":"conds"}},"key":"conds"}},{"kind":525,"flags":0,"lineno":758,"children":{"value":{"kind":513,"flags":0,"lineno":758,"children":{"expr":{"kind":256,"flags":0,"lineno":758,"children":{"name":"this"}},...

It seems unlikely (that's not meant to be live), but the timing fits.

Could this have been caused by T267859: Future of tests/phan in MW Core ?

Isn't it dead code?

Looking at the failure, seems like some code is using a non-literal key in getQueryInfo, which taint-check is not expecting. I'm investigating now, no guarantees to finish soon though. Please raise your voice if the world starts breaking for real.

Joy, this seems entirely deterministic, and so now the whole of WikiLambda codebase is unmergeable. Joy.

Change 641477 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@master] Skip non-literal keys in getQueryInfo

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

Change 641477 merged by jenkins-bot:
[mediawiki/tools/phan/SecurityCheckPlugin@master] Skip non-literal keys in getQueryInfo

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

Looking at the failure, seems like some code is using a non-literal key in getQueryInfo, which taint-check is not expecting.

Probably this https://gerrit.wikimedia.org/r/c/mediawiki/core/+/640667

Change 641304 had a related patch set uploaded (by Jforrester; owner: DannyS712):
[mediawiki/core@master] Follow-up I4034ba70e9: Adjust codestyle to make phan happy

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

Change 641481 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan@master] Bump taint-check to 3.0.4

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

Change 641488 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan@stable-0.10] Bump taint-check to 3.0.4

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

Change 641488 merged by jenkins-bot:
[mediawiki/tools/phan@stable-0.10] Bump taint-check to 3.0.4

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

Change 641481 merged by jenkins-bot:
[mediawiki/tools/phan@master] Bump taint-check to 3.0.4

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

Change 641304 merged by jenkins-bot:
[mediawiki/core@master] Follow-up I4034ba70e9: Adjust codestyle to make phan happy

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

OK, @DannyS712's quick hack merged in core to unbreak the world, and new release of mw-phan made and will be upgraded by LibUp. Thanks all!