See https://github.com/phan/phan/commit/c6ec98f605f38dd4f2a7bfac5800b7cb714ecfed
Given how taint-check processes method dependencies, this might catch more issues. I remember having tested the option with taint-check a few months ago, but can't recall what results I got.
| Subject | Author | Repo | Branch | Lines +/- | |
|---|---|---|---|---|---|
| Drop hacky/expensive code and recommend --analyze-twice instead. | Daimona Eaytoy | mediawiki/tools/phan/SecurityCheckPlugin | master | +170 -162 |
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Open | Daimona | T274780 Correctly process argument links when backpropagating EXEC taintedness | |||
| Open | Daimona | T281479 Implement NUMKEY propagation the proper way | |||
| Open | Daimona | T281478 Backpropagate method links without TaintednessBackpropVisitor | |||
| Resolved | Daimona | T281475 Avoid setting EXEC bits on variables | |||
| Resolved | Daimona | T269816 Investigate running taint-check with analyze-twice | |||
| Resolved | BUG REPORT | Daimona | T290515 Don't report duplicated issues with different caused-by lines |
the taint-check slows down phan a bit, I am not sure how much it is for twice analyze
analyze-twice is CLI only and cannot be set in the config (okay, you can set __analyze_twice but that seems not offical supported)
I do expect a slowdown, but I think it's also not very important. Performance is not the main priority here (although a performance improvement would indeed be great).
analyze-twice is CLI only and cannot be set in the config (okay, you can set __analyze_twice but that seems not offical supported)
We can set it in the CI docker image, or create an issue upstream if the lack of a config option is not intentional.
Quick analysis done at T203651#7031088. Comparison for MW core:
time: 2m 2s -> 2m 35s
memory: 3733 MB -> 4125 MB
+99 issues (some are duplicates, some are unused suppressions)
So I'd like to proceed in this direction, since --analyze-twice would allow us to simplify and speed up a couple of things in taint-check. Two things, for the record, that are currently close to, respectively, guessing which methods might need to be reanalyzed (T203651#7031088), and guessing that a variable will be EXECed without any regard to code placement (T281475).
Things that we might want to do first:
It's also unclear how strongly we want to recommend --analyze-twice (e.g. just suggest, or warmly recommend). In particular, what scenario should be assumed as the most common. And consequently, how to configure tests, e.g. keep non-twice and twice versions, or just run everything with --analyze-twice.
Change 683699 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@master] [WIP] Drop hacky/expensive code and recommend --analyze-twice instead.
Change 683699 merged by jenkins-bot:
[mediawiki/tools/phan/SecurityCheckPlugin@master] Drop hacky/expensive code and recommend --analyze-twice instead.
The README now suggests using analyze-twice for better analysis, with the caveat that some issues might be reported twice. The hacky/expensive code in taint-check that made some things hard to refactor (and was kinda trying to emulate analyze-twice) was removed, hence calling this resolved.