Skip to content

Conversation

smowton
Copy link
Contributor

@smowton smowton commented Feb 18, 2021

This adds models for WordUtils, StrTokenizer, StrLookup, StrSubstitutor, and all their equivalents in the Commons Text library.

The commits relating to StrBuilder are from #5172 which this PR is based upon; review only the latest 3 commits here.

@smowton smowton requested a review from a team as a code owner February 18, 2021 14:47
@github-actions github-actions bot added the Java label Feb 18, 2021
@smowton smowton force-pushed the smowton/feature/commons-misc-text branch from 38b7f10 to 399e646 Compare February 18, 2021 15:04
@smowton smowton force-pushed the smowton/feature/commons-misc-text branch from 399e646 to db5cd63 Compare March 2, 2021 11:41
@smowton
Copy link
Contributor Author

smowton commented Mar 2, 2021

@aschackmull I have converted this to use CSV specs, and slightly generalised the format to simplify describing generic types when we don't care to distinguish different specialsiations.

@smowton
Copy link
Contributor Author

smowton commented Mar 3, 2021

Now includes @joefarebrother's commit 41b7db1 to permit array types in signatures

@@ -279,7 +279,7 @@ private predicate elementSpec(
bindingset[namespace, type, subtypes]
private RefType interpretType(string namespace, string type, boolean subtypes) {
exists(RefType t |
t.hasQualifiedName(namespace, type) and
[t, t.getSourceDeclaration()].hasQualifiedName(namespace, type) and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the wrong place to put .getSourceDeclaration(). Instead we should change the 3 instances of ref.(Call).getCallee() = e to ref.(Call).getCallee().getSourceDeclaration() = e below in sourceElementRef, sinkElementRef, and summaryElementRef. With this change we'll track much fewer tuples in the intermediate predicates. It also allows specifying signatures when type variables are mentioned, as the current solution doesn't work for that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -2,6 +2,7 @@

import java
private import semmle.code.java.dataflow.FlowSteps
private import semmle.code.java.dataflow.ExternalFlow
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need bidirectional import. Currently that happens indirectly, but I think it's best to import this file directly in the private module Frameworks in ExternalFlow.qll.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@smowton smowton force-pushed the smowton/feature/commons-misc-text branch from 62903a7 to 71cd329 Compare March 4, 2021 11:13
@aschackmull aschackmull merged commit 3565ba5 into github:main Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants