From 098afb935bcec51d420bfd7d86e5cf9fa6eb98e5 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Thu, 14 Dec 2023 09:48:45 +0100 Subject: [PATCH] Address more review comments --- .../csharp/dataflow/internal/ExternalFlow.qll | 2 + go/ql/lib/semmle/go/dataflow/ExternalFlow.qll | 2 + .../code/java/dataflow/ExternalFlow.qll | 2 + .../dataflow/internal/AccessPathSyntax.qll | 4 +- .../dataflow/internal/FlowSummaryImpl.qll | 50 +++++-------------- .../codeql/swift/dataflow/ExternalFlow.qll | 2 + 6 files changed, 22 insertions(+), 40 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll index 2c54408c62b..90d8b6b0ecd 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll @@ -516,6 +516,7 @@ private predicate interpretSummary( ) } +// adapter class for converting Mad summaries to `SummarizedCallable`s private class SummarizedCallableAdapter extends SummarizedCallable { SummarizedCallableAdapter() { interpretSummary(this, _, _, _, _) } @@ -549,6 +550,7 @@ private class SummarizedCallableAdapter extends SummarizedCallable { } } +// adapter class for converting Mad neutrals to `NeutralCallable`s private class NeutralCallableAdapter extends NeutralCallable { string kind; string provenance_; diff --git a/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll b/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll index 1b3257405cd..cacad869509 100644 --- a/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll +++ b/go/ql/lib/semmle/go/dataflow/ExternalFlow.qll @@ -384,6 +384,7 @@ private predicate interpretSummary( ) } +// adapter class for converting Mad summaries to `SummarizedCallable`s private class SummarizedCallableAdapter extends SummarizedCallable { SummarizedCallableAdapter() { interpretSummary(this, _, _, _, _) } @@ -417,6 +418,7 @@ private class SummarizedCallableAdapter extends SummarizedCallable { } } +// adapter class for converting Mad neutrals to `NeutralCallable`s private class NeutralCallableAdapter extends NeutralCallable { string kind; string provenance_; diff --git a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll index 63a70ad436e..d2ce3d0a7d6 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll @@ -513,6 +513,7 @@ private module Cached { import Cached +// adapter class for converting Mad summaries to `SummarizedCallable`s private class SummarizedCallableAdapter extends SummarizedCallable { SummarizedCallableAdapter() { summaryElement(this, _, _, _, _) } @@ -546,6 +547,7 @@ private class SummarizedCallableAdapter extends SummarizedCallable { } } +// adapter class for converting Mad neutrals to `NeutralCallable`s private class NeutralCallableAdapter extends NeutralCallable { string kind; string provenance_; diff --git a/shared/dataflow/codeql/dataflow/internal/AccessPathSyntax.qll b/shared/dataflow/codeql/dataflow/internal/AccessPathSyntax.qll index 8c7d5b1e072..17b979e42a6 100644 --- a/shared/dataflow/codeql/dataflow/internal/AccessPathSyntax.qll +++ b/shared/dataflow/codeql/dataflow/internal/AccessPathSyntax.qll @@ -37,7 +37,7 @@ bindingset[arg] int parseLowerBound(string arg) { result = arg.regexpCapture("(-?\\d+)\\.\\.", 1).toInt() } /** - * An access part token such as `Argument[1]` or `ReturnValue`. + * An access path token such as `Argument[1]` or `ReturnValue`. */ class AccessPathTokenBase extends string { bindingset[this] @@ -181,7 +181,7 @@ module AccessPath { } /** - * An access part token such as `Argument[1]` or `ReturnValue`, appearing in one or more access paths. + * An access path token such as `Argument[1]` or `ReturnValue`, appearing in one or more access paths. */ class AccessPathToken extends AccessPathTokenBaseFinal { AccessPathToken() { this = getRawToken(_, _) } diff --git a/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll b/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll index ea200a28b8d..25a276d41a7 100644 --- a/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll +++ b/shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll @@ -394,16 +394,12 @@ module Make Input> { } } - private predicate summaryElement( - SummarizedCallable c, string input, string output, boolean preservesValue, string provenance - ) { - c.propagatesFlow(input, output, preservesValue) and - c.hasProvenance(provenance) - } - private predicate summarySpec(string spec) { - summaryElement(_, spec, _, _, _) or - summaryElement(_, _, spec, _, _) + exists(SummarizedCallable c | + c.propagatesFlow(spec, _, _) + or + c.propagatesFlow(_, spec, _) + ) } import AccessPathSyntax::AccessPath @@ -652,7 +648,7 @@ module Make Input> { /** * Holds if `c` has a flow summary from `input` to `arg`, where `arg` - * writes to (contents of) arguments at position `pos`, and `c` has a + * writes to (contents of) arguments at (some) position `pos`, and `c` has a * value-preserving flow summary from the arguments at position `pos` * to a return value (`return`). * @@ -1423,43 +1419,21 @@ module Make Input> { } } - private class SummarizedCallableExternal extends SummarizedCallableImpl instanceof SummarizedCallable + // adapter class for converting `SummarizedCallable`s to `SummarizedCallableImpl`s + private class SummarizedCallableImplAdapter extends SummarizedCallableImpl instanceof SummarizedCallable { - SummarizedCallableExternal() { summaryElement(this, _, _, _, _) } - - private predicate relevantSummaryElementGenerated( - AccessPath inSpec, AccessPath outSpec, boolean preservesValue - ) { - exists(Provenance provenance | - provenance.isGenerated() and - summaryElement(this, inSpec, outSpec, preservesValue, provenance) - ) and - not super.applyManualModel() - } - - private predicate relevantSummaryElement( - AccessPath inSpec, AccessPath outSpec, boolean preservesValue - ) { - exists(Provenance provenance | - provenance.isManual() and - summaryElement(this, inSpec, outSpec, preservesValue, provenance) - ) - or - this.relevantSummaryElementGenerated(inSpec, outSpec, preservesValue) - } - override predicate propagatesFlow( SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue ) { exists(AccessPath inSpec, AccessPath outSpec | - this.relevantSummaryElement(inSpec, outSpec, preservesValue) and + SummarizedCallable.super.propagatesFlow(inSpec, outSpec, preservesValue) and interpretSpec(inSpec, input) and interpretSpec(outSpec, output) ) } override predicate hasProvenance(Provenance provenance) { - summaryElement(this, _, _, _, provenance) + SummarizedCallable.super.hasProvenance(provenance) } } @@ -1492,13 +1466,13 @@ module Make Input> { /** * Holds if an external source specification exists for `n` with output specification - * `output`, kind `kind`, and provenance `provenance`. + * `output` and kind `kind`. */ predicate sourceElement(Element n, string output, string kind); /** * Holds if an external sink specification exists for `n` with input specification - * `input`, kind `kind` and provenance `provenance`. + * `input` and kind `kind`. */ predicate sinkElement(Element n, string input, string kind); diff --git a/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll b/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll index 4d95f1345a9..35515cb548c 100644 --- a/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll +++ b/swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll @@ -527,6 +527,7 @@ private predicate interpretSummary( ) } +// adapter class for converting Mad summaries to `SummarizedCallable`s private class SummarizedCallableAdapter extends SummarizedCallable { SummarizedCallableAdapter() { interpretSummary(this, _, _, _, _) } @@ -560,6 +561,7 @@ private class SummarizedCallableAdapter extends SummarizedCallable { } } +// adapter class for converting Mad neutrals to `NeutralCallable`s private class NeutralCallableAdapter extends NeutralCallable { string kind; string provenance_;