Java: replace the dead String.valueOf(CharSequence) summary with a synthetic callable - #22315
Conversation
…a synthetic callable for valueOf(Object) on CharSequence arguments Co-authored-by: hvitved <3667920+hvitved@users.noreply.github.com>
Co-authored-by: hvitved <3667920+hvitved@users.noreply.github.com>
String.valueOf(CharSequence) summary with a synthetic callable
hvitved
left a comment
There was a problem hiding this comment.
Fix the test failure reported by CI.
There was a problem hiding this comment.
Pull request overview
Replaces an ineffective String.valueOf(CharSequence) model with conditional propagation for String.valueOf(Object) calls receiving CharSequence values.
Changes:
- Adds and registers the synthetic callable.
- Removes the nonexistent-overload model.
- Adds regression tests, generated expectations, and a change note.
Show a summary per file
| File | Description |
|---|---|
java/ql/test/utils/modelgenerator/dataflow/p/Joiner.java |
Updates generated summary expectations. |
java/ql/test/library-tests/dataflow/string-valueof/test.ql |
Configures the flow test. |
java/ql/test/library-tests/dataflow/string-valueof/test.expected |
Records expected models and flow edges. |
java/ql/test/library-tests/dataflow/string-valueof/A.java |
Covers propagating and non-propagating argument types. |
java/ql/lib/semmle/code/java/frameworks/Strings.qll |
Defines conditional valueOf(Object) propagation. |
java/ql/lib/semmle/code/java/dataflow/FlowSummary.qll |
Registers the synthetic callable. |
java/ql/lib/ext/java.lang.model.yml |
Removes the dead summary row. |
java/ql/lib/change-notes/2026-08-11-string-valueof-charsequence.md |
Documents the analysis improvement. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 8/8 changed files
- Comments generated: 0
- Review effort level: Balanced
Rerun has been triggered: 2 restarted 🚀 |
owen-mc
left a comment
There was a problem hiding this comment.
Looks good. I didn't know about Synthetic Callable before. I've restarted the CI runs which failed - I suspect they are unrelated. If they pass then you can merge.
|
I've started DCA, so we should also wait for that to finish and check the results. |
Thanks, but I already did one. |
|
Ah yes, I missed that. The times seems okay (within the margins of error for normal java behaviour). And hive seems to lose one concatenated-sql alert and gain two sql-injection alerts. Did you investigate the alert changes? |
|
Yes, the alert changes looked good to me.
Sent from Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: Owen Mansel-Chan ***@***.***>
Sent: Thursday, 13 August 2026 16:20:13
To: github/codeql ***@***.***>
Cc: Tom Hvitved ***@***.***>; Assign ***@***.***>
Subject: Re: [github/codeql] Java: replace the dead `String.valueOf(CharSequence)` summary with a synthetic callable (PR #22315)
[https://avatars.githubusercontent.com/u/62447351?s=20&v=4]owen-mc left a comment (github/codeql#22315)<#22315?email_source=notifications&email_token=AA37PUFKYJTC5UFRLVB6THD5JXFB3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMRYGE3DMMJZGQYKM4TFMFZW63VGMFZXG2LHN2SWK5TFNZ2LK4DSL5RW63LNMVXHIX3POBSW4X3DNRUWG2Y#issuecomment-5281661940>
Ah yes, I missed that. The times seems okay (within the margins of error for normal java behaviour). And hive seems to lose one concatenated-sql alert and gain two sql-injection alerts. Did you investigate the alert changes?
—
Reply to this email directly, view it on GitHub<#22315?email_source=notifications&email_token=AA37PUFKYJTC5UFRLVB6THD5JXFB3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMRYGE3DMMJZGQYKM4TFMFZW63VGMFZXG2LHN2SWK5TFNZ2LK4DSL5RW63LNMVXHIX3POBSW4X3DNRUWG2Y#issuecomment-5281661940>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AA37PUFA5NUEW3T7Z2PW7SD5JXFB3AVCNFSNUABFKJSXA33TNF2G64TZHMYTIMZQGQYDIMRYHNEXG43VMU5TKMJRHAYDOOJUGI42C5QC>.
You are receiving this because you were assigned.Message ID: ***@***.***>
|
|
Cool. Well, it looks good to me and CI is passing. The only thing stopping it being merged is your review. |
java.lang.Stringhas novalueOf(CharSequence)overload — string- andCharSequence-typed arguments compile tovalueOf(Object), which is modelled as neutral. The summary row therefore matched no call site, leaving no propagatingvalueOfmodel for string-typed arguments:Changes
java/ql/lib/ext/java.lang.model.yml: removed the["java.lang", "String", False, "valueOf", "(CharSequence)", ...]row, which matched nothing.java/ql/lib/semmle/code/java/frameworks/Strings.qll(new): aSyntheticCallablethat propagates taintArgument[0]→ReturnValueforString.valueOf(Object)calls whose argument's static type is aCharSequence(String,CharSequence,StringBuilder, …), where the call is equivalent to the documentedCharSequence.toString(). Registered in theSyntheticCallablesmodule inFlowSummary.qll.valueOf(Object)neutral model is kept, so arguments of arbitrary types — whosetoString()may not expose tainted state — remain unaffected.java/ql/test/library-tests/dataflow/string-valueofcoversString,CharSequence,StringBuilder,char[]andchararguments (flow expected) and anObject-typed argument (no flow expected). Change note added.