Skip to content

C#: Tolerate missing call targets in LogMessageSink #14855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 22, 2023
Merged

C#: Tolerate missing call targets in LogMessageSink #14855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f0e20fa
C#: Add test case for missing log message sinks with ambiguous types
tamasvajk Nov 21, 2023
253c658
C#: Tolerate missing call targets in `LogMessageSink`
tamasvajk Nov 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/ExternalLocationSink.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@ private class ExternalModelSink extends ExternalLocationSink {
*/
class LogMessageSink extends ExternalLocationSink {
LogMessageSink() {
this.getExpr() = any(LoggerType i).getAMethod().getACall().getAnArgument()
or
this.getExpr() = any(LoggerType i).getAMethod().getACall().getAnArgument() or
this.getExpr() =
any(MethodCall call | call.getQualifier().getType() instanceof LoggerType).getAnArgument() or
this.getExpr() =
any(ExtensionMethodCall call |
call.getTarget().(ExtensionMethod).getExtendedType() instanceof LoggerType
Expand Down
8 changes: 8 additions & 0 deletions csharp/ql/test/library-tests/standalone/externalLocationSink/externalLocationSink.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#select
| standalone.cs:20:20:20:20 | access to parameter s | standalone.cs:20:20:20:20 | access to parameter s |
| standalone.cs:25:28:25:32 | "abc" | standalone.cs:25:28:25:32 | "abc" |
compilationErrors
| standalone.cs:16:12:16:18 | CS0104: 'ILogger' is an ambiguous reference between 'A.ILogger' and 'B.ILogger' |
methodCalls
| standalone.cs:20:9:20:21 | call to method |
| standalone.cs:25:9:25:33 | call to method |
10 changes: 10 additions & 0 deletions csharp/ql/test/library-tests/standalone/externalLocationSink/externalLocationSink.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import semmle.code.csharp.security.dataflow.flowsinks.ExternalLocationSink
import semmle.code.csharp.commons.Diagnostics

from ExternalLocationSink sink
where sink.getLocation().getFile().fromSource()
select sink, sink.getExpr()

query predicate compilationErrors(CompilerError e) { any() }

query predicate methodCalls(MethodCall m) { any() }
1 change: 1 addition & 0 deletions csharp/ql/test/library-tests/standalone/externalLocationSink/options
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
semmle-extractor-options: --standalone
27 changes: 27 additions & 0 deletions csharp/ql/test/library-tests/standalone/externalLocationSink/standalone.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
using A;
using B;

namespace A
{
public interface ILogger { }
}

namespace B
{
public interface ILogger { }
}

public class C
{
public ILogger logger;
Copy link
Contributor

Choose a reason for hiding this comment

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

OOI, which type does Roslyn then assign to this field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case it's A.ILogger. What's odd is that when we process the field, there's no sign of ambiguity.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's also what I don't understand, I would have expected that Roslyn didn't assign any type at all. Or is it perhaps because we just take the first candidate symbol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To conclude, we found that there's no ambiguity when processing the field or the reference to the field. But the field's type symbol is actually an ambiguous error symbol.


private void M(string s)
{
logger.Log(s);
}

private static void Main()
{
new C().logger.Log("abc");
}
}