Skip to content

Commit 9d247d6

Browse files
hvitvedhvitved
authored
Merge pull request #22315 from github/copilot/fix-valueof-call-site-issue
Java: replace the dead `String.valueOf(CharSequence)` summary with a synthetic callable
2 parents a80d7e0 + 7e3c144 commit 9d247d6

8 files changed

Lines changed: 115 additions & 1 deletion

File tree

‎java/ql/lib/change-notes/2026-08-11-string-valueof-charsequence.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Removed the summary model for `String.valueOf(CharSequence)`, which does not exist. Instead, taint is now propagated through calls to `String.valueOf(Object)` when the argument is a `CharSequence`, for example a `String` or a `StringBuilder`.

‎java/ql/lib/ext/java.lang.model.yml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,6 @@ extensions:
134134
- ["java.lang", "String", False, "valueOf", "(char)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
135135
- ["java.lang", "String", False, "valueOf", "(char[])", "", "Argument[0]", "ReturnValue", "taint", "manual"]
136136
- ["java.lang", "String", False, "valueOf", "(char[],int,int)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
137-
- ["java.lang", "String", False, "valueOf", "(CharSequence)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
138137
- ["java.lang", "StringBuffer", True, "StringBuffer", "(CharSequence)", "", "Argument[0]", "Argument[this]", "taint", "manual"]
139138
- ["java.lang", "StringBuffer", True, "StringBuffer", "(String)", "", "Argument[0]", "Argument[this]", "taint", "manual"]
140139
- ["java.lang", "StringBuilder", True, "StringBuilder", "", "", "Argument[0]", "Argument[this]", "taint", "manual"]

‎java/ql/lib/semmle/code/java/dataflow/FlowSummary.qll‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ private module SyntheticCallables {
4444
private import semmle.code.java.dispatch.WrappedInvocation
4545
private import semmle.code.java.frameworks.android.Intent
4646
private import semmle.code.java.frameworks.Stream
47+
private import semmle.code.java.frameworks.Strings
4748
}
4849

4950
private newtype TSummarizedCallableBase =

‎java/ql/lib/semmle/code/java/frameworks/Strings.qll‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
/** Definitions related to `java.lang.String`. */
2+
overlay[local?]
3+
module;
4+
5+
private import java
6+
private import semmle.code.java.dataflow.FlowSummary
7+
8+
/**
9+
* A call to `String.valueOf(Object)` where the argument is a `CharSequence`,
10+
* for example a `String` or a `StringBuilder`.
11+
*
12+
* Such a call is equivalent to calling `toString()` on the argument, which for
13+
* a `CharSequence` is guaranteed to yield a string containing the characters of
14+
* the argument, so taint is propagated. This is in contrast to `valueOf(Object)`
15+
* calls in general, where `toString()` may not expose the state of the argument.
16+
*/
17+
private class StringValueOfCharSequence extends SyntheticCallable {
18+
StringValueOfCharSequence() { this = "java.lang.String.valueOf(Object)+CharSequence" }
19+
20+
override MethodCall getACall() {
21+
exists(Method m | m = result.getMethod().getSourceDeclaration() |
22+
m.hasQualifiedName("java.lang", "String", "valueOf") and
23+
m.getParameterType(0) instanceof TypeObject
24+
) and
25+
result
26+
.getArgument(0)
27+
.getType()
28+
.(RefType)
29+
.getAnAncestor()
30+
.hasQualifiedName("java.lang", "CharSequence")
31+
}
32+
33+
override predicate propagatesFlow(string input, string output, boolean preservesValue) {
34+
input = "Argument[0]" and
35+
output = "ReturnValue" and
36+
preservesValue = false
37+
}
38+
39+
override Type getReturnType() { result instanceof TypeString }
40+
}

‎java/ql/test/library-tests/dataflow/string-valueof/A.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
public class A {
2+
String source() { return "source"; }
3+
4+
void sink(Object o) {}
5+
6+
void m() {
7+
String s = source();
8+
9+
sink(String.valueOf(s)); // $ hasTaintFlow
10+
11+
CharSequence seq = s;
12+
sink(String.valueOf(seq)); // $ hasTaintFlow
13+
14+
StringBuilder sb = new StringBuilder(s);
15+
sink(String.valueOf(sb)); // $ hasTaintFlow
16+
17+
sink(String.valueOf(s.toCharArray())); // $ hasTaintFlow
18+
19+
sink(String.valueOf(s.charAt(0))); // $ hasTaintFlow
20+
21+
// `toString` on an arbitrary object is not assumed to expose the state of
22+
// the object, so no flow is expected here.
23+
Object o = s;
24+
sink(String.valueOf(o));
25+
}
26+
}

‎java/ql/test/library-tests/dataflow/string-valueof/test.expected‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
models
2+
| 1 | Summary: java.lang; CharSequence; true; charAt; ; ; Argument[this]; ReturnValue; taint; manual |
3+
| 2 | Summary: java.lang; String; false; toCharArray; ; ; Argument[this]; ReturnValue; taint; manual |
4+
| 3 | Summary: java.lang; String; false; valueOf; (char); ; Argument[0]; ReturnValue; taint; manual |
5+
| 4 | Summary: java.lang; String; false; valueOf; (char[]); ; Argument[0]; ReturnValue; taint; manual |
6+
| 5 | Summary: java.lang; StringBuilder; true; StringBuilder; ; ; Argument[0]; Argument[this]; taint; manual |
7+
edges
8+
| A.java:7:16:7:23 | source(...) : String | A.java:9:25:9:25 | s : String | provenance | |
9+
| A.java:7:16:7:23 | source(...) : String | A.java:12:25:12:27 | seq : String | provenance | |
10+
| A.java:7:16:7:23 | source(...) : String | A.java:14:42:14:42 | s : String | provenance | |
11+
| A.java:7:16:7:23 | source(...) : String | A.java:17:25:17:25 | s : String | provenance | |
12+
| A.java:7:16:7:23 | source(...) : String | A.java:19:25:19:25 | s : String | provenance | |
13+
| A.java:9:25:9:25 | s : String | A.java:9:10:9:26 | valueOf(...) | provenance | java.lang.String.valueOf(Object)+CharSequence |
14+
| A.java:12:25:12:27 | seq : String | A.java:12:10:12:28 | valueOf(...) | provenance | java.lang.String.valueOf(Object)+CharSequence |
15+
| A.java:14:24:14:43 | new StringBuilder(...) : StringBuilder | A.java:15:25:15:26 | sb : StringBuilder | provenance | |
16+
| A.java:14:42:14:42 | s : String | A.java:14:24:14:43 | new StringBuilder(...) : StringBuilder | provenance | MaD:5 |
17+
| A.java:15:25:15:26 | sb : StringBuilder | A.java:15:10:15:27 | valueOf(...) | provenance | java.lang.String.valueOf(Object)+CharSequence |
18+
| A.java:17:25:17:25 | s : String | A.java:17:25:17:39 | toCharArray(...) : char[] | provenance | MaD:2 |
19+
| A.java:17:25:17:39 | toCharArray(...) : char[] | A.java:17:10:17:40 | valueOf(...) | provenance | MaD:4 |
20+
| A.java:19:25:19:25 | s : String | A.java:19:25:19:35 | charAt(...) : Number | provenance | MaD:1 |
21+
| A.java:19:25:19:35 | charAt(...) : Number | A.java:19:10:19:36 | valueOf(...) | provenance | MaD:3 |
22+
nodes
23+
| A.java:7:16:7:23 | source(...) : String | semmle.label | source(...) : String |
24+
| A.java:9:10:9:26 | valueOf(...) | semmle.label | valueOf(...) |
25+
| A.java:9:25:9:25 | s : String | semmle.label | s : String |
26+
| A.java:12:10:12:28 | valueOf(...) | semmle.label | valueOf(...) |
27+
| A.java:12:25:12:27 | seq : String | semmle.label | seq : String |
28+
| A.java:14:24:14:43 | new StringBuilder(...) : StringBuilder | semmle.label | new StringBuilder(...) : StringBuilder |
29+
| A.java:14:42:14:42 | s : String | semmle.label | s : String |
30+
| A.java:15:10:15:27 | valueOf(...) | semmle.label | valueOf(...) |
31+
| A.java:15:25:15:26 | sb : StringBuilder | semmle.label | sb : StringBuilder |
32+
| A.java:17:10:17:40 | valueOf(...) | semmle.label | valueOf(...) |
33+
| A.java:17:25:17:25 | s : String | semmle.label | s : String |
34+
| A.java:17:25:17:39 | toCharArray(...) : char[] | semmle.label | toCharArray(...) : char[] |
35+
| A.java:19:10:19:36 | valueOf(...) | semmle.label | valueOf(...) |
36+
| A.java:19:25:19:25 | s : String | semmle.label | s : String |
37+
| A.java:19:25:19:35 | charAt(...) : Number | semmle.label | charAt(...) : Number |
38+
subpaths
39+
testFailures

‎java/ql/test/library-tests/dataflow/string-valueof/test.ql‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
import utils.test.InlineFlowTest
2+
import DefaultFlowTest
3+
import TaintFlow::PathGraph

‎java/ql/test/utils/modelgenerator/dataflow/p/Joiner.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ public String toString() {
8282
}
8383

8484
// heuristic-summary=p;Joiner;false;add;(CharSequence);;Argument[this];ReturnValue;value;df-generated
85+
// heuristic-summary=p;Joiner;false;add;(CharSequence);;Argument[0];Argument[this];taint;df-generated
8586
// contentbased-summary=p;Joiner;false;add;(CharSequence);;Argument[this];ReturnValue;value;dfc-generated
8687
// MISSING content based summaries for "elts". This could be a synthetic field.
8788
public Joiner add(CharSequence newElement) {
@@ -107,6 +108,7 @@ private int checkAddLength(int oldLen, int inc) {
107108
}
108109

109110
// heuristic-summary=p;Joiner;false;merge;(Joiner);;Argument[this];ReturnValue;value;df-generated
111+
// heuristic-summary=p;Joiner;false;merge;(Joiner);;Argument[0];Argument[this];taint;df-generated
110112
// contentbased-summary=p;Joiner;false;merge;(Joiner);;Argument[this];ReturnValue;value;dfc-generated
111113
// MISSING content based summaries for "elts". This could be a synthetic field.
112114
public Joiner merge(Joiner other) {

0 commit comments

Comments
 (0)