Page MenuHomePhabricator

phan-taint-check-plugin depends on creation of dynamic properties which gets deprecated in php8.2
Closed, ResolvedPublicBUG REPORT

Description

phan-taint-check-plugin fails on php8.2 with deprecation notice of dynamic properties on upstream classes.

More infos at https://wiki.php.net/rfc/deprecate_dynamic_properties

Upstream: https://github.com/phan/phan/issues/4776

Note: As of phan 5.4.1 the analyze of php8.2 is not supported
(Phan 5 supports analyzing PHP version 7.0-8.1 syntax)

Example when running phan on codesniffer repo - https://integration.wikimedia.org/ci/job/composer-package-php82-docker/12/console

/src/vendor/mediawiki/phan-taint-check-plugin/src/TaintednessAccessorsTrait.php:196 [8192] Creation of dynamic property Phan\Language\Element\Method::$funcTaint is deprecated
(Phan 5.4.1 crashed when parsing/analyzing 'MediaWiki/Sniffs/AlternativeSyntax/AlternativeSyntaxSniff.php')
More details:
#2: SecurityCheckPlugin\TaintednessVisitor::doSetFuncTaint() called at [/src/vendor/mediawiki/phan-taint-check-plugin/src/TaintednessBaseVisitor.php:87] Args: [Phan\Language\Element\Method(\MediaWiki\Sniffs\AlternativeSyntax\AlternativeSyntaxSniff::register()), SecurityCheckPlugin\FunctionTaintedness({})]
#3: SecurityCheckPlugin\TaintednessVisitor->addFuncTaint() called at [/src/vendor/mediawiki/phan-taint-check-plugin/src/TaintednessVisitor.php:969] Args: [Phan\Language\Element\Method(\MediaWiki\Sniffs\AlternativeSyntax\AlternativeSyntaxSniff::register()), SecurityCheckPlugin\FunctionTaintedness({})]

Example from plugin itself - https://integration.wikimedia.org/ci/job/composer-package-php82-docker/13/console

/src/src/TaintednessAccessorsTrait.php:39 [8192] Creation of dynamic property Phan\Language\Element\Parameter::$taintedness is deprecated
(Phan 5.4.1 crashed when parsing/analyzing 'integration/backpropnumkey-conservative/db.php')
More details:
#2: SecurityCheckPlugin\PreTaintednessVisitor::setTaintednessRaw() called at [/src/src/PreTaintednessVisitor.php:100] Args: [Phan\Language\Element\Parameter($table), SecurityCheckPlugin\Taintedness({})]
#3: SecurityCheckPlugin\PreTaintednessVisitor->visitMethod() called at [/src/src/MWPreVisitor.php:35] Args: [ast\Node({"kind":69,"flags":1,"lineno":5,"children":{"name":"select","docComment":null,"params":{"kind":136,"flags":0,"lineno":6,"children":[{"kind":1280,"flags":0,"lineno":6,"children":{"type":null,"name":"table","default":null,"attributes":null,"docComment":null}},{"kind":1280,"flags":0,"lineno":6,"children":{"type":null,"name":"vars","default":null,"attributes":null,"docComment":null}},{"kind":1280,"flags":0,"lineno":6,"children":{"type":null,"name":"conds","default":"","attributes":null,"docComment":null}},{"kind":1280,"flags":0,"lineno":6,"children":{"type":null,"name":"fname","default":{"kind":0,"flags":348,"lineno":6,"children":[]},"attributes":null,"docComment":null}},{"kind":1280,"flags":0,"lineno":7,"children":{"type":null,"name":"options","default":{"kind":129,"flags":3,"lineno":7,"children":[]},"attributes":null,"docComment":null}},{"kind":1280,"flags":0,"lineno":7,"chil...

Event Timeline

Umherirrender changed the task status from Open to Stalled.Dec 16 2022, 2:00 PM

Soooo this is something I've been aware of since the dynamic properties RFC came out. The easiest solution, if upstream agree, would be to add #[AllowDynamicProperties] to the phan classes that we add dynamic properties to (TypedElement and UnaddressableTypedElement would suffice). This would make the warning go away and essentially preserve the status quo. For the long term, I've always wanted to check with upstream if we could do something to avoid the creation of dynamic properties. I've also always suspected that using dynamic properties made taint-check slower, although this doesn't seem to be too true, judging from some quick tests with PHP 8.2. Maybe one option could be to have a generic array $pluginData property in the relevant phan classes, and we would put our data there. OTOH, I'm not sure if it should be implemented as a method, because method calls are slower and these properties can be accessed tens of thousands of times when analyzing core.

DooDee renamed this task from phan-taint-check-plugin depends on creation of dynamic properties which gets deprecated in php8.2 to Thailand.th.Apr 5 2023, 10:07 AM
DooDee triaged this task as Medium priority.
DooDee updated the task description. (Show Details)
DooDee updated Other Assignee, added: DooDee.
DooDee removed subscribers: Daimona, Aklapper, Umherirrender.
Mainframe98 renamed this task from Thailand.th to phan-taint-check-plugin depends on creation of dynamic properties which gets deprecated in php8.2.Apr 5 2023, 10:21 AM
Mainframe98 raised the priority of this task from Medium to Needs Triage.
Mainframe98 updated the task description. (Show Details)
Mainframe98 updated Other Assignee, removed: DooDee.

I've filed https://github.com/phan/phan/issues/4776 upstream. https://github.com/phan/phan/issues/4614 is about phan itself, and I thought having a separate issue for plugins would be better.

The minimum-thinking workaround to dynamic properties is to keep a hashmap of object -> extra properties, either with spl_object_id() or SplObjectStorage (WeakMap if you are in PHP 8+).

The minimum-thinking workaround to dynamic properties is to keep a hashmap of object -> extra properties, either with spl_object_id() or SplObjectStorage (WeakMap if you are in PHP 8+).

I remember trying this a long time ago, but unfortunately it wouldn't work because the data would be lost when the phan object is cloned. At any given time, there can be multiple phan objects referencing the same element (variable, property, etc.); for instance, when analyzing an if-else branch, phan makes a copiy of the involved variables for each branch, and then merges the mutations seen by the copies into a single object at the end of the conditional (or something like that, I forgot exactly how it works). In this and similar circumstances, we need our taintedness data to survive the clone, which is the case when storing the data on the object itself, but not when using an object-based storage.

But even if it did work, I'd be concerned about the performance of using a huge map, given the number of objects that get created when analyzing a large code base.

@Krinkle made a PR that would solve all our issues, the question is if and when it's going to be reviewed...

The PR has been merged by Rasmus. Now it's just ("just"?) a matter of waiting for the next release.

Mainframe98 changed the task status from Stalled to Open.Feb 8 2024, 8:06 PM
Mainframe98 subscribed.

Let's reflect the status quo with https://gerrit.wikimedia.org/r/c/mediawiki/tools/phan/SecurityCheckPlugin/+/989246 being merged (or does that completely resolve this task?)

Let's reflect the status quo with https://gerrit.wikimedia.org/r/c/mediawiki/tools/phan/SecurityCheckPlugin/+/989246 being merged (or does that completely resolve this task?)

I hope that it will be fixed once we've rolled out the new version of mediawiki/tools/phan (v0.14.0) to all repos; it's slow going so far: https://gerrit.wikimedia.org/r/q/hashtag:c%25E2%2580%2594mediawiki%252Fmediawiki-phan-config%253D0.14.0+(status:open%20OR%20status:merged)

Let's reflect the status quo with https://gerrit.wikimedia.org/r/c/mediawiki/tools/phan/SecurityCheckPlugin/+/989246 being merged (or does that completely resolve this task?)

I hope that it will be fixed once we've rolled out the new version of mediawiki/tools/phan (v0.14.0) to all repos; it's slow going so far: https://gerrit.wikimedia.org/r/q/hashtag:c%25E2%2580%2594mediawiki%252Fmediawiki-phan-config%253D0.14.0+(status:open%20OR%20status:merged)

Indeed. I guess LibUp is still processing the huge backlog. Also, it's currently choking on both AbuseFilter and UploadWizard (canary repos) because of T356567. I thought of doing the upgrade manually, but then again, I'm not sure if it's going to make much of a difference because of the backlog, assuming that the stylelint issue will be figured out soon.

Let's reflect the status quo with https://gerrit.wikimedia.org/r/c/mediawiki/tools/phan/SecurityCheckPlugin/+/989246 being merged (or does that completely resolve this task?)

I hope that it will be fixed once we've rolled out the new version of mediawiki/tools/phan (v0.14.0) to all repos; it's slow going so far: https://gerrit.wikimedia.org/r/q/hashtag:c%25E2%2580%2594mediawiki%252Fmediawiki-phan-config%253D0.14.0+(status:open%20OR%20status:merged)

Indeed. I guess LibUp is still processing the huge backlog. Also, it's currently choking on both AbuseFilter and UploadWizard (canary repos) because of T356567. I thought of doing the upgrade manually, but then again, I'm not sure if it's going to make much of a difference because of the backlog, assuming that the stylelint issue will be figured out soon.

I've made some manual commits just for phan for those two repos and a handful of others; that'll unblock the canary run and so open out the upgrades to everywhere.