Swift: Modernize the swift/string-length-conflation query #12642
Conversation
… what really happens.
| class StringLengthConflationConfiguration extends TaintTracking::Configuration { | ||
| StringLengthConflationConfiguration() { this = "StringLengthConflationConfiguration" } | ||
|
|
||
| override predicate isSource(DataFlow::Node node, string flowstate) { |
There was a problem hiding this comment.
Once Swift starts converting dataflow configurations to modules this flowstate could be simplified to a newtype with 5 unit branches.
There was a problem hiding this comment.
Yes, I noticed we will benefit from the new configurations here. I've added a note of your exact suggestion to https://github.com/github/codeql-c-team/issues/1625.
|
We lose an alert on DCA. I need to look into this. Performance is fine. |
|
Fixed the regression. For some reason none of the string views work well expressed in MaD, so they are now all defined in QL. Not ideal, but its difficult to investigate further as these are built-in classes. I also did a MRVA top 1000 run for the base + new top of this branch to check for any other regressions. We lose 5 FPs and gain 2 TPs, I found no other changes. |
|
DCA LGTM. |
Modernize the
swift/string-length-conflationquery:Note that the existing weaknesses in the definitions of sources and sinks in this query become more explicit here (references to too general classes
Collection,Sequenceetc appear that were implicit before), but on the positive side they're actually slightly more constrained now than before. I've created an issue to find a way to make this better: https://github.com/github/codeql-c-team/issues/1655I've tested this on the MRVA top 100 - revealing a few false positive results that have been fixed by the slightly more precise sinks. I've also added some test cases inspired by that.
I will do a DCA run as well...