taint-check fails on array-plus and assumed int|float type
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Umherirrender
	Nov 27 2020, 4:20 PM

Description

I have tested mediawiki/phan-taint-check-plugin 3.1.0 against core and it now shows:

float|int passed to foreach instead of array

for following code in PPTemplateFrame_Hash.php

		$args = $this->numberedArgs + $this->namedArgs;
		foreach ( $args as $name => $value ) {
			if ( $first ) {
				$first = false;
			} else {
				$s .= ', ';
			}
			$s .= "\"$name\":\"" .
				str_replace( '"', '\\"', $value->__toString() ) . '"';
		}

It is possible that taint is not assume the corret type here with the array plus?
There is no @var on the class properties, but taint could should be strong about the fact?
Phan without taint does not warn on this foreach.

Details

	Project	Branch	Lines +/-	Subject
	mediawiki/tools/phan/SecurityCheckPlugin	master	+21 -0	Add tests for PhanTypeMismatchForeach edge case
	mediawiki/tools/phan/SecurityCheckPlugin	master	+16 -10	Fix bug which made variables lose their type if overridden in branches

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Resolved	Daimona	T203630 Configure CI to run phan-taint-check-plugin for MediaWiki core
Resolved	Umherirrender	T231311 Enable seccheck for MW core
Resolved	None	T216348 Suppress or fix non-double escape phan-taint-check warnings for MW core
Resolved	Daimona	T268891 taint-check fails on array-plus and assumed int\|float type

Event Timeline

Umherirrender created this task.Nov 27 2020, 4:20 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 27 2020, 4:20 PM

Umherirrender added a parent task: T216348: Suppress or fix non-double escape phan-taint-check warnings for MW core.Nov 27 2020, 4:32 PM

It is possible that taint is not assume the corret type here with the array plus?

It is possible, both areas (foreach loops and type inference) were touched between 3.0.4 and 3.1.0. I'm looking.

Not sure if related, but also related to types:

Language.php

Expected an object instance when accessing an instance property, but saw an expression $ts with type string

	private function getHumanTimestampInternal(
		MWTimestamp $ts, MWTimestamp $relativeTo, User $user
	) {
[...]
		if ( $diff->invert || $days > 5
			&& $ts->timestamp->format( 'Y' ) !== $relativeTo->timestamp->format( 'Y' )
		) {

$ts is typehinted and taint lost the type

ResourceLoader.php

Expected an object instance when accessing an instance property, but saw an expression $scripts with type null

		if ( $scripts instanceof XmlJsCode ) {
			if ( $scripts->value === '' ) {  # <!-- issue on this line
				$scripts = null;

It was checked with instanceof

Parser.php

Expected an object instance when accessing an instance property, but saw an expression $this->mPreprocessor with type null

		# Fix cloning
		if ( isset( $this->mPreprocessor ) && $this->mPreprocessor->parser !== $this ) {
			$this->mPreprocessor = null;
		}

Isset should be a null check as well

Opened https://github.com/phan/phan/issues/4271 for the original issue. Checking the others...

Change 643990 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@master] [WIP] PhanTypeMismatchForeach

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

gerritbot added a project: Patch-For-Review.Nov 27 2020, 10:52 PM

Daimona mentioned this in T268903: Run phan's test suite as part of CI.Nov 27 2020, 10:55 PM

Change 643992 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@master] Fix bug which made variables lose their type if overridden in branches

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

In T268891#6653088, @Umherirrender wrote:

Not sure if related, but also related to types:

Language.php [...]
ResourceLoader.php [...]
Parser.php [...]

These were caused by the same bug: trying to analyze the if condition after the body. All of those ifs change the type of a variable used in the condition, which resulted in an error. Fixed by avoiding the analysis of the if condition altogether, because redundant and wrong (which should also have the nice side effect of improving performance).

As per link above, this is an upstream issue.

Change 643992 merged by jenkins-bot:
[mediawiki/tools/phan/SecurityCheckPlugin@master] Fix bug which made variables lose their type if overridden in branches

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

Daimona mentioned this in rMTPSb4f511fde70a: Fix bug which made variables lose their type if overridden in branches.Dec 11 2020, 8:24 PM

Stalled because the remaining issue should be resolved upstream.

DannyS712 moved this task from Backlog to Reported Upstream on the Upstream board.Jun 6 2021, 7:48 AM

Umherirrender mentioned this in T289751: Update from Phan 3 to Phan 4 (or Phan 5).Aug 26 2021, 7:50 PM

Our part is done. I'm not sure if the upstream issue is going to be fixed: since the addition of numbers is more common than array addition, phan infers number-like types for the operands of + when the types are unknown. Changing that might have unwanted side effects. Since this only caused a single issue in MW core that was promptly suppressed, I believe it's unnecessary to keep this task open.

Change 643990 abandoned by Daimona Eaytoy:

[mediawiki/tools/phan/SecurityCheckPlugin@master] Add tests for PhanTypeMismatchForeach edge case

Reason:

It's an upstream bug, so no need to document it here.

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

Maintenance_bot removed a project: Patch-For-Review.Sep 7 2021, 9:10 AM

taint-check fails on array-plus and assumed int|float typeClosed, ResolvedPublicActions

Description

Details

Related ObjectsSearch...

Event Timeline

taint-check fails on array-plus and assumed int|float type
Closed, ResolvedPublic
Actions

Related Objects
Search...