C#: Adding synthetic implicit ToString calls in binary- and string interpolation expressions. #18446

michaelnebel · 2025-01-08T13:16:00Z

In this PR we get the ball rolling on synthesizing compiler generated ToString calls.
We introduce ToString calls in binary expressions for concatenating strings and string interpolation expressions:
That is,

"abc" + x -> "abc" + x.ToString()`
$"abx{x}" -> $"abx{x.ToString()}"

This is expected to increase accuracy of the data flow analysis, but there are some pitfalls:

We might loose results in case, the ToString that is implicitly called is not in source code and if we don't have a model for it.
We might get more results: If Object.ToString is the one being implicitly called and there exists a ToString implementation that contains a source for a query. In this case we might get false positives due to dynamic dispatch. DCA reports a couple of results like that (cs/web/xss, cs/log-forging and cs/information-exposure-through-exception on ASP.NET and cs/cleartext-storage-of-sensitive-information and cs/exposure-of-sensitive-information on mono). In any case, this is an anti pattern that should generally be avoided (there is also a quality query for this). We could consider not to extract the implicit to string call when the target is ToString is on System.Object?

The extra result on ASP.NET for cs/web/unvalidated-url-redirection is due to the summary model added for PathString.ToString.

According to DCA til change does not affect performance.

github-actions · 2025-01-13T12:06:47Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

csharp

Generated file changes for csharp

Changes to framework-coverage-csharp.rst:

-    Others,"``Amazon.Lambda.APIGatewayEvents``, ``Amazon.Lambda.Core``, ``Dapper``, ``ILCompiler``, ``ILLink.RoslynAnalyzer``, ``ILLink.Shared``, ``ILLink.Tasks``, ``Internal.IL``, ``Internal.Pgo``, ``Internal.TypeSystem``, ``JsonToItemsTaskFactory``, ``Microsoft.Android.Build``, ``Microsoft.Apple.Build``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.AspNetCore.Components``, ``Microsoft.AspNetCore.Mvc``, ``Microsoft.AspNetCore.WebUtilities``, ``Microsoft.CSharp``, ``Microsoft.Diagnostics.Tools.Pgo``, ``Microsoft.DotNet.Build.Tasks``, ``Microsoft.DotNet.PlatformAbstractions``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.Diagnostics.Metrics``, ``Microsoft.Extensions.FileProviders``, ``Microsoft.Extensions.FileSystemGlobbing``, ``Microsoft.Extensions.Hosting``, ``Microsoft.Extensions.Http``, ``Microsoft.Extensions.Logging``, ``Microsoft.Extensions.Options``, ``Microsoft.Extensions.Primitives``, ``Microsoft.Interop``, ``Microsoft.JSInterop``, ``Microsoft.NET.Build.Tasks``, ``Microsoft.NET.Sdk.WebAssembly``, ``Microsoft.NET.WebAssembly.Webcil``, ``Microsoft.VisualBasic``, ``Microsoft.WebAssembly.Build.Tasks``, ``Microsoft.Win32``, ``Mono.Linker``, ``MySql.Data.MySqlClient``, ``Newtonsoft.Json``, ``SourceGenerators``, ``Windows.Security.Cryptography.Core``",61,2074,152,4
+    Others,"``Amazon.Lambda.APIGatewayEvents``, ``Amazon.Lambda.Core``, ``Dapper``, ``ILCompiler``, ``ILLink.RoslynAnalyzer``, ``ILLink.Shared``, ``ILLink.Tasks``, ``Internal.IL``, ``Internal.Pgo``, ``Internal.TypeSystem``, ``JsonToItemsTaskFactory``, ``Microsoft.Android.Build``, ``Microsoft.Apple.Build``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.AspNetCore.Components``, ``Microsoft.AspNetCore.Http``, ``Microsoft.AspNetCore.Mvc``, ``Microsoft.AspNetCore.WebUtilities``, ``Microsoft.CSharp``, ``Microsoft.Diagnostics.Tools.Pgo``, ``Microsoft.DotNet.Build.Tasks``, ``Microsoft.DotNet.PlatformAbstractions``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.Diagnostics.Metrics``, ``Microsoft.Extensions.FileProviders``, ``Microsoft.Extensions.FileSystemGlobbing``, ``Microsoft.Extensions.Hosting``, ``Microsoft.Extensions.Http``, ``Microsoft.Extensions.Logging``, ``Microsoft.Extensions.Options``, ``Microsoft.Extensions.Primitives``, ``Microsoft.Interop``, ``Microsoft.JSInterop``, ``Microsoft.NET.Build.Tasks``, ``Microsoft.NET.Sdk.WebAssembly``, ``Microsoft.NET.WebAssembly.Webcil``, ``Microsoft.VisualBasic``, ``Microsoft.WebAssembly.Build.Tasks``, ``Microsoft.Win32``, ``Mono.Linker``, ``MySql.Data.MySqlClient``, ``Newtonsoft.Json``, ``SourceGenerators``, ``Windows.Security.Cryptography.Core``",61,2075,152,4
-    Totals,,108,12900,400,9
+    Totals,,108,12901,400,9

Changes to framework-coverage-csharp.csv:

+ Microsoft.AspNetCore.Http,,,1,,,,,,,,,,,,,,,,,,,1,

Copilot reviewed 35 out of 50 changed files in this pull request and generated no comments.

Files not reviewed (15)

csharp/ql/examples/snippets/ternary_conditional.ql: Language not supported
csharp/ql/lib/semmle/code/csharp/PrintAst.qll: Language not supported
csharp/ql/lib/semmle/code/csharp/commons/Constants.qll: Language not supported
csharp/ql/lib/semmle/code/csharp/commons/Strings.qll: Language not supported
csharp/ql/lib/semmle/code/csharp/dataflow/Nullness.qll: Language not supported
csharp/ql/lib/semmle/code/csharp/dispatch/Dispatch.qll: Language not supported
csharp/ql/lib/semmle/code/csharp/exprs/Call.qll: Language not supported
csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll: Language not supported
csharp/ql/src/Bad Practices/VirtualCallInConstructorOrDestructor.ql: Language not supported
csharp/ql/src/Likely Bugs/Dynamic/BadDynamicCall.ql: Language not supported
csharp/ql/src/Likely Bugs/ObjectComparison.ql: Language not supported
csharp/ql/test/library-tests/controlflow/graph/BasicBlock.expected: Language not supported
csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs: Evaluated as low risk
csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/InterpolatedString.cs: Evaluated as low risk
csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/ImplicitCast.cs: Evaluated as low risk

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

…ring calls into account.

…ons and generated ToString calls.

…rences.

…l calls.

…ons, if the type implements IFormattable.

tamasvajk

LGTM

hvitved

Great work; one question.

hvitved · 2025-01-16T10:31:33Z

csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/ImplicitToString.cs

+        private static IMethodSymbol? GetToStringMethod(ITypeSymbol? type)
+        {
+            return type?
+                .GetMembers()


Does this include inherited members?

No, it doesn't.

Maybe we should consider to support this as well - should be fairly easy.

Also good observation @hvitved !! 👍

Maybe we should consider to support this as well - should be fairly easy.

Yes, I think we should support that as well.

Follow up #18508

github-actions bot added the C# label Jan 8, 2025

michaelnebel force-pushed the csharp/implicittostring2 branch 2 times, most recently from 7ba7af1 to 9959976 Compare January 9, 2025 10:43

github-actions bot added the documentation label Jan 9, 2025

michaelnebel force-pushed the csharp/implicittostring2 branch 7 times, most recently from bf46c42 to 825c64f Compare January 10, 2025 13:17

michaelnebel changed the title ~~C#: Adding synthetic implicit ToString calls in binary expressions.~~ C#: Adding synthetic implicit ToString calls in binary- and string interpolation expressions. Jan 10, 2025

michaelnebel mentioned this pull request Jan 10, 2025

C#: Introduce synthetic ToString calls where appropriate. #18442

Closed

michaelnebel force-pushed the csharp/implicittostring2 branch from 825c64f to 9ebee34 Compare January 10, 2025 13:51

michaelnebel marked this pull request as ready for review January 14, 2025 10:07

Copilot bot review requested due to automatic review settings January 14, 2025 10:07

michaelnebel requested a review from a team as a code owner January 14, 2025 10:07

Copilot AI reviewed Jan 14, 2025

View reviewed changes

michaelnebel added 11 commits January 14, 2025 17:13

C#: Add some implicit toString data flow test cases.

261b8db

C#: Add a RedundantToStringCall test case.

f905be4

C#: Make synthetic ToString calls in binary add expressions.

908a3e3

C#: Update test expected output.

e9beeac

C#: Make synthetic ToString calls in string interpolation expressions.

5f539c5

C#: Update test expected output.

8bf67e3

C#: Modify compiler generated strip logic to also take generated ToSt…

a4049b1

…ring calls into account.

C#: Add PrintAst test to implicit ToString test.

6c6e58b

C#: Update AST printing to include generated cast (wrapping) expressi…

53c2f76

…ons and generated ToString calls.

C#: Update PrintAst expected test output.

f239ab1

C#: Update other existing tests expected output.

20c2b2b

michaelnebel added 10 commits January 14, 2025 17:14

C#: Add change note.

cd7d2d4

C#: Add ImplicitToString example in nullness tests.

6a31fd7

C#: Disregard compiler generated method calls as possible null derefe…

e62846e

…rences.

C#: Update expected testoutput for Nullness tests.

d40c1be

C#: Add implicit to string usage in constructor.

77f9f4c

C#: Disregards compiler generated virtual calls as problematic virtua…

2bff2d6

…l calls.

C#: Update virtual call in constructor expected test output.

024de5e

C#: Add a summary model for PathString.ToString.

ab70a94

C#: Do not insert a synthetic ToString call in interpolation expressi…

6a406b2

…ons, if the type implements IFormattable.

C#: Add implicit to string test.

0c5c2a3

michaelnebel force-pushed the csharp/implicittostring2 branch from dbc7a80 to 0c5c2a3 Compare January 14, 2025 16:17

C#: Update other existing tests expected output.

3de5b22

michaelnebel marked this pull request as draft January 16, 2025 08:08

michaelnebel marked this pull request as ready for review January 16, 2025 08:08

tamasvajk approved these changes Jan 16, 2025

View reviewed changes

michaelnebel merged commit ba2b7ab into github:main Jan 16, 2025
22 checks passed

hvitved reviewed Jan 16, 2025

View reviewed changes

michaelnebel deleted the csharp/implicittostring2 branch January 16, 2025 10:37

michaelnebel mentioned this pull request Jan 16, 2025

TaintFlow not picking up on implicit toString() #18399

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C#: Adding synthetic implicit ToString calls in binary- and string interpolation expressions. #18446

C#: Adding synthetic implicit ToString calls in binary- and string interpolation expressions. #18446

michaelnebel commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025

csharp

Generated file changes for csharp

tamasvajk left a comment

hvitved left a comment

hvitved Jan 16, 2025

michaelnebel Jan 16, 2025

michaelnebel Jan 16, 2025

michaelnebel Jan 16, 2025

hvitved Jan 16, 2025

michaelnebel Jan 16, 2025

C#: Adding synthetic implicit ToString calls in binary- and string interpolation expressions. #18446

C#: Adding synthetic implicit ToString calls in binary- and string interpolation expressions. #18446

Conversation

michaelnebel commented Jan 8, 2025 • edited Loading

github-actions bot commented Jan 13, 2025

csharp

Generated file changes for csharp

Choose a reason for hiding this comment

tamasvajk left a comment

Choose a reason for hiding this comment

hvitved left a comment

Choose a reason for hiding this comment

hvitved Jan 16, 2025

Choose a reason for hiding this comment

michaelnebel Jan 16, 2025

Choose a reason for hiding this comment

michaelnebel Jan 16, 2025

Choose a reason for hiding this comment

michaelnebel Jan 16, 2025

Choose a reason for hiding this comment

hvitved Jan 16, 2025

Choose a reason for hiding this comment

michaelnebel Jan 16, 2025

Choose a reason for hiding this comment

michaelnebel commented Jan 8, 2025 •

edited

Loading