Skip to content
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.

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

C#/Java/Go: Neutrals are split into separate classes. #17007

Merged
merged 4 commits into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 0 additions & 18 deletions cpp/ql/lib/semmle/code/cpp/dataflow/ExternalFlow.qll
Original file line number Diff line number Diff line change
Expand Up @@ -934,21 +934,3 @@ private class SummarizedCallableAdapter extends SummarizedCallable {
interpretSummary(this, _, _, _, provenance)
}
}

// adapter class for converting Mad neutrals to `NeutralCallable`s
private class NeutralCallableAdapter extends NeutralCallable {
string kind;
string provenance_;

NeutralCallableAdapter() {
// Neutral models have not been implemented for CPP.
none() and
exists(this) and
exists(kind) and
exists(provenance_)
}

override string getKind() { result = kind }

override predicate hasProvenance(Provenance provenance) { provenance = provenance_ }
}
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,7 @@ private predicate interpretSummary(
)
}

private predicate interpretNeutral(UnboundCallable c, string kind, string provenance) {
predicate interpretNeutral(UnboundCallable c, string kind, string provenance) {
exists(string namespace, string type, string name, string signature |
neutralModel(namespace, type, name, signature, kind, provenance) and
c = interpretElement(namespace, type, false, name, signature, "")
Expand Down Expand Up @@ -613,18 +613,6 @@ private class SummarizedCallableAdapter extends SummarizedCallable {
}
}

// adapter class for converting Mad neutrals to `NeutralCallable`s
private class NeutralCallableAdapter extends NeutralCallable {
string kind;
string provenance_;

NeutralCallableAdapter() { interpretNeutral(this, kind, provenance_) }

override string getKind() { result = kind }

override predicate hasProvenance(Provenance provenance) { provenance = provenance_ }
}

/**
* A callable where there exists a MaD sink model that applies to it.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ private import semmle.code.csharp.dataflow.internal.ExternalFlow
module Input implements InputSig<Location, DataFlowImplSpecific::CsharpDataFlow> {
class SummarizedCallableBase = UnboundCallable;

predicate neutralElement(SummarizedCallableBase c, string kind, string provenance, boolean isExact) {
interpretNeutral(c, kind, provenance) and
// isExact is not needed for C#.
isExact = false
}

ArgumentPosition callbackSelfParameterPosition() { result.isDelegateSelf() }

ReturnKind getStandardReturnValueKind() { result instanceof NormalReturnKind }
Expand Down
10 changes: 5 additions & 5 deletions csharp/ql/test/library-tests/dataflow/library/FlowSummaries.ql
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import shared.FlowSummaries
import semmle.code.csharp.dataflow.internal.ExternalFlow

final private class NeutralCallableFinal = NeutralCallable;

class RelevantNeutralCallable extends NeutralCallableFinal {
final string getCallableCsv() { result = getSignature(this) }
module R implements RelevantNeutralCallableSig<NeutralSummaryCallable> {
class RelevantNeutralCallable extends NeutralSummaryCallable {
final string getCallableCsv() { result = getSignature(this) }
}
}

class RelevantSourceCallable extends SourceCallable {
Expand All @@ -16,5 +16,5 @@ class RelevantSinkCallable extends SinkCallable {
}

import TestSummaryOutput<IncludeSummarizedCallable>
import TestNeutralOutput<RelevantNeutralCallable>
import TestNeutralOutput<NeutralSummaryCallable, R>
import External::TestSourceSinkOutput<RelevantSourceCallable, RelevantSinkCallable>
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,5 @@ extensions:
extensible: neutralModel
data:
- [ "Models", "ManuallyModelled", "HasNeutralSummaryNoFlow", "(System.Object)", "summary", "manual"]
- [ "Sinks", "NewSinks", "NoSink", "(System.Object)", "summary", "df-generated"]
- [ "Sinks", "NewSinks", "NoSink", "(System.Object)", "sink", "manual"]
4 changes: 4 additions & 0 deletions csharp/ql/test/utils/modelgenerator/dataflow/Sinks.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ public class NewSinks
// neutral=Sinks;NewSinks;Sink2;(System.Object);summary;df-generated
public static void Sink2(object o) => throw null;

// Defined as sink neutral in the file next to the neutral summary test.
// neutral=Sinks;NewSinks;NoSink;(System.Object);summary;df-generated
public static void NoSink(object o) => throw null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Now the bug is fixed, isn't this a confusing name? (Same for the java test.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The method is defined as sink (manual) and summary (df-generated) neutral in the extensible file.
That is, we expect that the "neutral" test (which only reports summary neutrals) would generate a neutral (df-generated) summary for this method.
The name "NoSink" is because it is explicitly stated in the extensible file that this method is sink neutral (it is not a sink).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Thanks for explaining.


// New sink
// sink=Sinks;NewSinks;false;WrapResponseWrite;(System.Object);;Argument[0];html-injection;df-generated
// neutral=Sinks;NewSinks;WrapResponseWrite;(System.Object);summary;df-generated
Expand Down
12 changes: 0 additions & 12 deletions go/ql/lib/semmle/go/dataflow/ExternalFlow.qll
Original file line number Diff line number Diff line change
Expand Up @@ -595,15 +595,3 @@ private class SummarizedCallableAdapter extends SummarizedCallable {
summaryElement(this, _, _, _, provenance, _)
}
}

// adapter class for converting Mad neutrals to `NeutralCallable`s
private class NeutralCallableAdapter extends NeutralCallable {
string kind;
string provenance_;

NeutralCallableAdapter() { neutralElement(this, kind, provenance_) }

override string getKind() { result = kind }

override predicate hasProvenance(Provenance provenance) { provenance = provenance_ }
}
16 changes: 12 additions & 4 deletions go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,17 @@ private string positionToString(int pos) {
module Input implements InputSig<Location, DataFlowImplSpecific::GoDataFlow> {
class SummarizedCallableBase = Callable;

predicate neutralElement(
Input::SummarizedCallableBase c, string kind, string provenance, boolean isExact
) {
exists(string namespace, string type, string name, string signature |
neutralModel(namespace, type, name, signature, kind, provenance) and
c.asFunction() = interpretElement(namespace, type, false, name, signature, "").asEntity()
) and
// isExact is not needed for Go.
isExact = false
}

ArgumentPosition callbackSelfParameterPosition() { result = -1 }

ReturnKind getStandardReturnValueKind() { result = getReturnKind(0) }
Expand Down Expand Up @@ -304,10 +315,7 @@ module Private {
* and with provenance `provenance`.
*/
predicate neutralElement(Input::SummarizedCallableBase c, string kind, string provenance) {
exists(string namespace, string type, string name, string signature |
neutralModel(namespace, type, name, signature, kind, provenance) and
c.asFunction() = interpretElement(namespace, type, false, name, signature, "").asEntity()
)
Input::neutralElement(c, kind, provenance, _)
}
}

Expand Down
15 changes: 0 additions & 15 deletions java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll
Original file line number Diff line number Diff line change
Expand Up @@ -627,21 +627,6 @@ private class SummarizedCallableAdapter extends SummarizedCallable {
override predicate hasExactModel() { summaryElement(this, _, _, _, _, _, true) }
}

// adapter class for converting Mad neutrals to `NeutralCallable`s
private class NeutralCallableAdapter extends NeutralCallable {
string kind;
string provenance_;
boolean exact;

NeutralCallableAdapter() { neutralElement(this, kind, provenance_, exact) }

override string getKind() { result = kind }

override predicate hasProvenance(Provenance provenance) { provenance = provenance_ }

override predicate hasExactModel() { exact = true }
}

/**
* A callable where there exists a MaD sink model that applies to it.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ private string positionToString(int pos) {
module Input implements InputSig<Location, DataFlowImplSpecific::JavaDataFlow> {
class SummarizedCallableBase = FlowSummary::SummarizedCallableBase;

predicate neutralElement(
Input::SummarizedCallableBase c, string kind, string provenance, boolean isExact
) {
exists(string namespace, string type, string name, string signature |
neutralModel(namespace, type, name, signature, kind, provenance) and
c.asCallable() = interpretElement(namespace, type, false, name, signature, "", isExact)
)
}

ArgumentPosition callbackSelfParameterPosition() { result = -1 }

ReturnKind getStandardReturnValueKind() { any() }
Expand Down Expand Up @@ -332,18 +341,7 @@ module Private {
)
}

/**
* Holds if a neutral model exists for `c` of kind `kind`
* and with provenance `provenance`.
*/
predicate neutralElement(
Input::SummarizedCallableBase c, string kind, string provenance, boolean isExact
) {
exists(string namespace, string type, string name, string signature |
neutralModel(namespace, type, name, signature, kind, provenance) and
c.asCallable() = interpretElement(namespace, type, false, name, signature, "", isExact)
)
}
predicate neutralElement = Input::neutralElement/4;
}

/** Provides predicates for constructing summary components. */
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
extensions:
- addsTo:
pack: codeql/java-all
extensible: neutralModel
data:
- [ "p", "Sinks", "nosink", "(Object)", "sink", "manual"]
- [ "p", "Sinks", "nosink", "(Object)", "summary", "df-generated"]
4 changes: 4 additions & 0 deletions java/ql/test/utils/modelgenerator/dataflow/p/Sinks.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ public void sink(Object o) {}
// neutral=p;Sinks;sink2;(Object);summary;df-generated
public void sink2(Object o) {}

// Defined as sink neutral in the file next to the neutral summary test.
// neutral=p;Sinks;nosink;(Object);summary;df-generated
public void nosink(Object o) {}

// sink=p;Sinks;true;copyFileToDirectory;(Path,Path,CopyOption[]);;Argument[0];path-injection;df-generated
// sink=p;Sinks;true;copyFileToDirectory;(Path,Path,CopyOption[]);;Argument[1];path-injection;df-generated
// neutral=p;Sinks;copyFileToDirectory;(Path,Path,CopyOption[]);summary;df-generated
Expand Down
Loading
Loading