From 3a4f0299c728b9fc81f65405dd3ee68d606c9546 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 17 Jun 2022 15:15:25 +0200 Subject: [PATCH 01/14] fix typo --- javascript/ql/src/Expressions/TypoDatabase.qll | 2 ++ ql/ql/src/codeql_ql/ast/internal/Predicate.qll | 2 +- ql/ql/src/codeql_ql/style/TypoDatabase.qll | 2 ++ 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/Expressions/TypoDatabase.qll b/javascript/ql/src/Expressions/TypoDatabase.qll index aad43d9d0cc..fadc1b8af75 100644 --- a/javascript/ql/src/Expressions/TypoDatabase.qll +++ b/javascript/ql/src/Expressions/TypoDatabase.qll @@ -5793,6 +5793,8 @@ predicate typos(string wrong, string right) { or wrong = "paramters" and right = "parameters" or + wrong = "parametarized" and right = "parameterized" + or wrong = "paranthesis" and right = "parenthesis" or wrong = "paraphenalia" and right = "paraphernalia" diff --git a/ql/ql/src/codeql_ql/ast/internal/Predicate.qll b/ql/ql/src/codeql_ql/ast/internal/Predicate.qll index eb7e298b7cd..0e614c23c25 100644 --- a/ql/ql/src/codeql_ql/ast/internal/Predicate.qll +++ b/ql/ql/src/codeql_ql/ast/internal/Predicate.qll @@ -176,7 +176,7 @@ module PredConsistency { c > 1 and resolvePredicateExpr(pe, p) } - // This can happen with parametarized modules + // This can happen with parameterized modules /* * query predicate multipleResolveCall(Call call, int c, PredicateOrBuiltin p) { * c = diff --git a/ql/ql/src/codeql_ql/style/TypoDatabase.qll b/ql/ql/src/codeql_ql/style/TypoDatabase.qll index aad43d9d0cc..fadc1b8af75 100644 --- a/ql/ql/src/codeql_ql/style/TypoDatabase.qll +++ b/ql/ql/src/codeql_ql/style/TypoDatabase.qll @@ -5793,6 +5793,8 @@ predicate typos(string wrong, string right) { or wrong = "paramters" and right = "parameters" or + wrong = "parametarized" and right = "parameterized" + or wrong = "paranthesis" and right = "parenthesis" or wrong = "paraphenalia" and right = "paraphernalia" From a59f0d36f55fac849e2fa97d7db40dbddd929c2e Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 17 Jun 2022 18:06:12 +0200 Subject: [PATCH 02/14] run the implicit-this patch on QL-for-QL --- ql/ql/src/codeql_ql/ast/Ast.qll | 2 +- ql/ql/src/codeql_ql/dataflow/DataFlow.qll | 16 +++++++++------- ql/ql/src/queries/reports/FrameworkCoverage.ql | 6 ++++-- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/ql/ql/src/codeql_ql/ast/Ast.qll b/ql/ql/src/codeql_ql/ast/Ast.qll index fa551b0de83..ffd8d07cd2a 100644 --- a/ql/ql/src/codeql_ql/ast/Ast.qll +++ b/ql/ql/src/codeql_ql/ast/Ast.qll @@ -1167,7 +1167,7 @@ class Import extends TImport, ModuleMember, TypeRef { */ string getImportString() { exists(string selec | - not exists(getSelectionName(_)) and selec = "" + not exists(this.getSelectionName(_)) and selec = "" or selec = "::" + strictconcat(int i, string q | q = this.getSelectionName(i) | q, "::" order by i) diff --git a/ql/ql/src/codeql_ql/dataflow/DataFlow.qll b/ql/ql/src/codeql_ql/dataflow/DataFlow.qll index da8bc1da837..c9043416bae 100644 --- a/ql/ql/src/codeql_ql/dataflow/DataFlow.qll +++ b/ql/ql/src/codeql_ql/dataflow/DataFlow.qll @@ -204,7 +204,7 @@ class SuperNode extends LocalFlow::TSuperNode { Node getANode() { LocalFlow::getRepr(result) = repr } /** Gets an AST node from any of the nodes in this super node. */ - AstNode asAstNode() { result = getANode().asAstNode() } + AstNode asAstNode() { result = this.getANode().asAstNode() } /** * Gets a single node from this super node. @@ -214,23 +214,25 @@ class SuperNode extends LocalFlow::TSuperNode { * - An `AstNodeNode` is preferred over other nodes. * - A node occurring earlier is preferred over one occurring later. */ - Node getArbitraryRepr() { result = min(Node n | n = getANode() | n order by getInternalId(n)) } + Node getArbitraryRepr() { + result = min(Node n | n = this.getANode() | n order by getInternalId(n)) + } /** * Gets the predicate containing all nodes that are part of this super node. */ - Predicate getEnclosingPredicate() { result = getANode().getEnclosingPredicate() } + Predicate getEnclosingPredicate() { result = this.getANode().getEnclosingPredicate() } /** Gets a string representation of this super node. */ string toString() { exists(int c | - c = strictcount(getANode()) and - result = "Super node of " + c + " nodes in " + getEnclosingPredicate().getName() + c = strictcount(this.getANode()) and + result = "Super node of " + c + " nodes in " + this.getEnclosingPredicate().getName() ) } /** Gets the location of an arbitrary node in this super node. */ - Location getLocation() { result = getArbitraryRepr().getLocation() } + Location getLocation() { result = this.getArbitraryRepr().getLocation() } /** Gets any member call whose receiver is in the same super node. */ MemberCall getALocalMemberCall() { superNode(result.getBase()) = this } @@ -287,7 +289,7 @@ class SuperNode extends LocalFlow::TSuperNode { cached private string getAStringValue(Tracker t) { t.start() and - result = asAstNode().(String).getValue() + result = this.asAstNode().(String).getValue() or exists(SuperNode pred, Tracker t2 | this = pred.track(t2, t) and diff --git a/ql/ql/src/queries/reports/FrameworkCoverage.ql b/ql/ql/src/queries/reports/FrameworkCoverage.ql index 7f5eb7f5310..f394a5a0091 100644 --- a/ql/ql/src/queries/reports/FrameworkCoverage.ql +++ b/ql/ql/src/queries/reports/FrameworkCoverage.ql @@ -18,11 +18,13 @@ class PackageImportCall extends PredicateCall { PackageImportCall() { this.getQualifier().getName() = ["API", "DataFlow"] and this.getPredicateName() = ["moduleImport", "moduleMember"] and - not isExcludedFile(getLocation().getFile()) + not isExcludedFile(this.getLocation().getFile()) } /** Gets the name of a package referenced by this call */ - string getAPackageName() { result = DataFlow::superNode(getArgument(0)).getAStringValueNoCall() } + string getAPackageName() { + result = DataFlow::superNode(this.getArgument(0)).getAStringValueNoCall() + } } /** Gets a reference to `package` or any transitive member thereof. */ From 7e93416e9728ba24d2936027d033607c952621e1 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 17 Jun 2022 21:32:19 +0200 Subject: [PATCH 03/14] only resolve module types if we know that the TypeExpr could possibly resolve to a module --- ql/ql/src/codeql_ql/ast/Ast.qll | 13 +++++++++++-- ql/ql/test/queries/style/RedundantCast/Foo.qll | 11 +++++++++++ .../style/RedundantCast/RedundantCast.expected | 3 +++ .../queries/style/RedundantCast/RedundantCast.qlref | 1 + 4 files changed, 26 insertions(+), 2 deletions(-) create mode 100644 ql/ql/test/queries/style/RedundantCast/Foo.qll create mode 100644 ql/ql/test/queries/style/RedundantCast/RedundantCast.expected create mode 100644 ql/ql/test/queries/style/RedundantCast/RedundantCast.qlref diff --git a/ql/ql/src/codeql_ql/ast/Ast.qll b/ql/ql/src/codeql_ql/ast/Ast.qll index ffd8d07cd2a..09f6e35171f 100644 --- a/ql/ql/src/codeql_ql/ast/Ast.qll +++ b/ql/ql/src/codeql_ql/ast/Ast.qll @@ -682,8 +682,17 @@ class TypeExpr extends TType, TypeRef { // resolve type resolveTypeExpr(this, result) or - // if it resolves to a module - exists(FileOrModule mod | resolveModuleRef(this, mod) | result = mod.toType()) + // if it resolves to a module, + exists(FileOrModule mod | resolveModuleRef(this, mod) | result = mod.toType()) and + result instanceof ModuleType and + // we can get spurious results in some cases, so we restrict to where it is possible to have a module. + ( + // only possible if this is inside a moduleInstantiation. + this = any(ModuleExpr mod).getArgument(_).asType() + or + // or if it's a parameter to a parameterized module + this = any(SignatureExpr sig, Module mod | mod.hasParameter(_, _, sig) | sig).asType() + ) } override AstNode getAChild(string pred) { diff --git a/ql/ql/test/queries/style/RedundantCast/Foo.qll b/ql/ql/test/queries/style/RedundantCast/Foo.qll new file mode 100644 index 00000000000..d993f654bc4 --- /dev/null +++ b/ql/ql/test/queries/style/RedundantCast/Foo.qll @@ -0,0 +1,11 @@ +class Foo extends string { + Foo() { this = "Foo" } +} + +predicate test(Foo f) { f.(Foo).toString() = "X" } + +predicate test2(Foo a, Foo b) { a.(Foo) = b } + +predicate called(Foo a) { a.toString() = "X" } + +predicate test3(string s) { called(s.(Foo)) } diff --git a/ql/ql/test/queries/style/RedundantCast/RedundantCast.expected b/ql/ql/test/queries/style/RedundantCast/RedundantCast.expected new file mode 100644 index 00000000000..e4e57083633 --- /dev/null +++ b/ql/ql/test/queries/style/RedundantCast/RedundantCast.expected @@ -0,0 +1,3 @@ +| Foo.qll:5:25:5:31 | InlineCast | Redundant cast to $@ | Foo.qll:5:28:5:30 | TypeExpr | Foo | +| Foo.qll:7:33:7:39 | InlineCast | Redundant cast to $@ | Foo.qll:7:36:7:38 | TypeExpr | Foo | +| Foo.qll:11:36:11:42 | InlineCast | Redundant cast to $@ | Foo.qll:11:39:11:41 | TypeExpr | Foo | diff --git a/ql/ql/test/queries/style/RedundantCast/RedundantCast.qlref b/ql/ql/test/queries/style/RedundantCast/RedundantCast.qlref new file mode 100644 index 00000000000..659062d3ae5 --- /dev/null +++ b/ql/ql/test/queries/style/RedundantCast/RedundantCast.qlref @@ -0,0 +1 @@ +queries/style/RedundantCast.ql From 0391db67876892ef943108e44d52725f9dbbcfb5 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 17 Jun 2022 22:52:17 +0200 Subject: [PATCH 04/14] simplify some code based on review --- ql/ql/src/codeql_ql/ast/Ast.qll | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/ql/ql/src/codeql_ql/ast/Ast.qll b/ql/ql/src/codeql_ql/ast/Ast.qll index 09f6e35171f..a237e1f6b58 100644 --- a/ql/ql/src/codeql_ql/ast/Ast.qll +++ b/ql/ql/src/codeql_ql/ast/Ast.qll @@ -2226,9 +2226,7 @@ class ModuleExpr extends TModuleExpr, TypeRef { or not exists(me.getName()) and result = me.getChild().(QL::SimpleId).getValue() or - exists(QL::ModuleInstantiation instantiation | instantiation.getParent() = me | - result = instantiation.getName().getChild().getValue() - ) + result = me.getChild().(QL::ModuleInstantiation).getName().getChild().getValue() } /** @@ -2263,9 +2261,7 @@ class ModuleExpr extends TModuleExpr, TypeRef { * The result is either a `PredicateExpr` or a `TypeExpr`. */ SignatureExpr getArgument(int i) { - exists(QL::ModuleInstantiation instantiation | instantiation.getParent() = me | - result.toQL() = instantiation.getChild(i) - ) + result.toQL() = me.getChild().(QL::ModuleInstantiation).getChild(i) } } From 638a886dfef9d4601feca560df5225071a22e1a8 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Sat, 18 Jun 2022 16:43:56 +0200 Subject: [PATCH 05/14] move create-extractor-pack to a `scripts` folder --- .github/workflows/ql-for-ql-dataset_measure.yml | 2 +- .github/workflows/ql-for-ql-tests.yml | 2 +- ql/README.md | 4 ++-- ql/{ => scripts}/create-extractor-pack.ps1 | 0 ql/{ => scripts}/create-extractor-pack.sh | 0 5 files changed, 4 insertions(+), 4 deletions(-) rename ql/{ => scripts}/create-extractor-pack.ps1 (100%) rename ql/{ => scripts}/create-extractor-pack.sh (100%) diff --git a/.github/workflows/ql-for-ql-dataset_measure.yml b/.github/workflows/ql-for-ql-dataset_measure.yml index cf3b696f3b8..a5ed2e9b266 100644 --- a/.github/workflows/ql-for-ql-dataset_measure.yml +++ b/.github/workflows/ql-for-ql-dataset_measure.yml @@ -36,7 +36,7 @@ jobs: ql/target key: ${{ runner.os }}-qltest-cargo-${{ hashFiles('ql/**/Cargo.lock') }} - name: Build Extractor - run: cd ql; env "PATH=$PATH:`dirname ${CODEQL}`" ./create-extractor-pack.sh + run: cd ql; env "PATH=$PATH:`dirname ${CODEQL}`" ./scripts/create-extractor-pack.sh env: CODEQL: ${{ steps.find-codeql.outputs.codeql-path }} - name: Checkout ${{ matrix.repo }} diff --git a/.github/workflows/ql-for-ql-tests.yml b/.github/workflows/ql-for-ql-tests.yml index 3b0a4963b79..b016f21f2b9 100644 --- a/.github/workflows/ql-for-ql-tests.yml +++ b/.github/workflows/ql-for-ql-tests.yml @@ -36,7 +36,7 @@ jobs: run: | cd ql; codeqlpath=$(dirname ${{ steps.find-codeql.outputs.codeql-path }}); - env "PATH=$PATH:$codeqlpath" ./create-extractor-pack.sh + env "PATH=$PATH:$codeqlpath" ./scripts/create-extractor-pack.sh - name: Run QL tests run: | "${CODEQL}" test run --check-databases --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition --search-path "${{ github.workspace }}/ql/extractor-pack" --consistency-queries ql/ql/consistency-queries ql/ql/test diff --git a/ql/README.md b/ql/README.md index 4bb5e30ba84..29b5aaef63c 100644 --- a/ql/README.md +++ b/ql/README.md @@ -21,14 +21,14 @@ cargo build --release The generated `ql/src/ql.dbscheme` and `ql/src/codeql_ql/ast/internal/TreeSitter.qll` files are included in the repository, but they can be re-generated as follows: ```bash -./create-extractor-pack.sh +./scripts/create-extractor-pack.sh ``` ## Building a CodeQL database for a QL program First, get an extractor pack: -Run `./create-extractor-pack.sh` (Linux/Mac) or `.\create-extractor-pack.ps1` (Windows PowerShell) and the pack will be created in the `extractor-pack` directory. +Run `./scripts/create-extractor-pack.sh` (Linux/Mac) or `.\scripts\create-extractor-pack.ps1` (Windows PowerShell) and the pack will be created in the `extractor-pack` directory. Then run diff --git a/ql/create-extractor-pack.ps1 b/ql/scripts/create-extractor-pack.ps1 similarity index 100% rename from ql/create-extractor-pack.ps1 rename to ql/scripts/create-extractor-pack.ps1 diff --git a/ql/create-extractor-pack.sh b/ql/scripts/create-extractor-pack.sh similarity index 100% rename from ql/create-extractor-pack.sh rename to ql/scripts/create-extractor-pack.sh From 6e2f3e2fcb8097e48195491326f96e975587504e Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Sat, 18 Jun 2022 16:44:10 +0200 Subject: [PATCH 06/14] merge all .sarif files at the end of the QL-for-QL workflow --- .github/workflows/ql-for-ql-build.yml | 27 ++++++++++++++++++++++++--- ql/scripts/merge-sarif.js | 26 ++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 ql/scripts/merge-sarif.js diff --git a/.github/workflows/ql-for-ql-build.yml b/.github/workflows/ql-for-ql-build.yml index 6b4f6a0abee..ffb4ad1cbc0 100644 --- a/.github/workflows/ql-for-ql-build.yml +++ b/.github/workflows/ql-for-ql-build.yml @@ -50,9 +50,6 @@ jobs: path: ${{ runner.temp }}/query-pack.zip extractors: - strategy: - fail-fast: false - runs-on: ubuntu-latest steps: @@ -201,3 +198,27 @@ jobs: name: ${{ matrix.folder }}.sarif path: ${{ matrix.folder }}.sarif + combine: + runs-on: ubuntu-latest + needs: + - analyze + + steps: + - uses: actions/checkout@v3 + - name: Make a folder for artifacts. + run: mkdir -p results + - name: Download all sarif files + uses: actions/download-artifact@v3 + with: + path: results + - uses: actions/setup-node@v3 + with: + node-version: 16 + - name: Combine all sarif files + run: | + node ./ql/scripts/merge-sarif.js results/**/*.sarif combined.sarif + - name: Upload combined sarif file + uses: actions/upload-artifact@v3 + with: + name: combined.sarif + path: combined.sarif diff --git a/ql/scripts/merge-sarif.js b/ql/scripts/merge-sarif.js new file mode 100644 index 00000000000..c7507986d5a --- /dev/null +++ b/ql/scripts/merge-sarif.js @@ -0,0 +1,26 @@ +var fs = require("fs"); + +// first a list of files to merge, and the last argument is the output file. +async function main(files) { + const inputs = files + .slice(0, -1) + .map((file) => fs.readFileSync(file)) + .map((data) => JSON.parse(data)); + const out = inputs[0]; // just arbitrarily take the first one + const outFile = files[files.length - 1]; + + const combinedResults = []; + + for (const sarif of inputs) { + combinedResults.push(...sarif.runs[0].results); + } + + out.runs[0].artifacts = []; // the indexes in these won't make sense, so I hope this works. + out.runs[0].results = combinedResults; + + // workaround until https://github.com/microsoft/sarif-vscode-extension/pull/436/ is part of a release + out["$schema"] = "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0"; + + fs.writeFileSync(outFile, JSON.stringify(out, null, 2)); +} +main(process.argv.splice(2)); From 1856e2b3891c6b2e534057bdd763332208729aa3 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Sun, 19 Jun 2022 13:12:38 +0200 Subject: [PATCH 07/14] fixup the $schema in all .sarif files --- .github/workflows/ql-for-ql-build.yml | 3 +++ ql/scripts/merge-sarif.js | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ql-for-ql-build.yml b/.github/workflows/ql-for-ql-build.yml index ffb4ad1cbc0..95b93dd772c 100644 --- a/.github/workflows/ql-for-ql-build.yml +++ b/.github/workflows/ql-for-ql-build.yml @@ -192,6 +192,9 @@ jobs: category: "ql-for-ql-${{ matrix.folder }}" - name: Copy sarif file to CWD run: cp ../results/ql.sarif ./${{ matrix.folder }}.sarif + - name: Fixup the $scema in sarif # Until https://github.com/microsoft/sarif-vscode-extension/pull/436/ is part in a stable release + run: | + sed -i 's/\$schema.*/\$schema": "https:\/\/raw.githubusercontent.com\/oasis-tcs\/sarif-spec\/master\/Schemata\/sarif-schema-2.1.0",/' ${{ matrix.folder }}.sarif - name: Sarif as artifact uses: actions/upload-artifact@v3 with: diff --git a/ql/scripts/merge-sarif.js b/ql/scripts/merge-sarif.js index c7507986d5a..5c781f1b67d 100644 --- a/ql/scripts/merge-sarif.js +++ b/ql/scripts/merge-sarif.js @@ -18,9 +18,6 @@ async function main(files) { out.runs[0].artifacts = []; // the indexes in these won't make sense, so I hope this works. out.runs[0].results = combinedResults; - // workaround until https://github.com/microsoft/sarif-vscode-extension/pull/436/ is part of a release - out["$schema"] = "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0"; - fs.writeFileSync(outFile, JSON.stringify(out, null, 2)); } main(process.argv.splice(2)); From 26df367a8a9b6be00fcabed22e9f0c352e167cfe Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Sun, 19 Jun 2022 18:59:00 +0200 Subject: [PATCH 08/14] fix some instances of spuriously resolving to multiple predicates --- .../src/codeql_ql/ast/internal/Predicate.qll | 7 +++-- ql/ql/test/callgraph/MultiResolve.qll | 26 +++++++++++++++++++ ql/ql/test/callgraph/callgraph.expected | 5 ++++ 3 files changed, 36 insertions(+), 2 deletions(-) create mode 100644 ql/ql/test/callgraph/MultiResolve.qll diff --git a/ql/ql/src/codeql_ql/ast/internal/Predicate.qll b/ql/ql/src/codeql_ql/ast/internal/Predicate.qll index 0e614c23c25..b4a5eaa6574 100644 --- a/ql/ql/src/codeql_ql/ast/internal/Predicate.qll +++ b/ql/ql/src/codeql_ql/ast/internal/Predicate.qll @@ -31,7 +31,8 @@ private predicate definesPredicate( name = alias.getName() and resolvePredicateExpr(alias.getAlias(), p) and public = getPublicBool(alias) and - arity = alias.getArity() + arity = alias.getArity() and + arity = p.getArity() ) ) } @@ -53,10 +54,12 @@ private module Cached { } private predicate resolvePredicateCall(PredicateCall pc, PredicateOrBuiltin p) { + // calls to class methods exists(Class c, ClassType t | c = pc.getParent*() and t = c.getType() and - p = t.getClassPredicate(pc.getPredicateName(), pc.getNumberOfArguments()) + p = t.getClassPredicate(pc.getPredicateName(), pc.getNumberOfArguments()) and + not exists(pc.getQualifier()) // no module qualifier, because then it's not a call to a class method. ) or exists(FileOrModule m, boolean public | diff --git a/ql/ql/test/callgraph/MultiResolve.qll b/ql/ql/test/callgraph/MultiResolve.qll new file mode 100644 index 00000000000..161d86077ad --- /dev/null +++ b/ql/ql/test/callgraph/MultiResolve.qll @@ -0,0 +1,26 @@ +predicate foo(int a, int b) { + a = 2 and + b = 2 +} + +predicate foo(int a, int b, int c) { + a = 2 and + b = 2 and + c = 2 +} + +predicate myFoo = foo/2; + +predicate test(int i) { myFoo(i, i) } // <- should only resolve to the `foo` with 2 arguments (and the `myFoo` alias). + +module MyMod { + predicate bar() { any() } + + class Bar extends int { + Bar() { this = 42 } + + predicate bar() { + MyMod::bar() // <- should resolve to the module's predicate. + } + } +} diff --git a/ql/ql/test/callgraph/callgraph.expected b/ql/ql/test/callgraph/callgraph.expected index 97c84034602..b640afaecc6 100644 --- a/ql/ql/test/callgraph/callgraph.expected +++ b/ql/ql/test/callgraph/callgraph.expected @@ -14,6 +14,9 @@ getTarget | Foo.qll:31:5:31:12 | PredicateCall | Foo.qll:24:3:24:32 | ClasslessPredicate alias0 | | Foo.qll:36:36:36:65 | MemberCall | file://:0:0:0:0 | replaceAll | | Foo.qll:38:39:38:67 | MemberCall | file://:0:0:0:0 | regexpCapture | +| MultiResolve.qll:14:25:14:35 | PredicateCall | MultiResolve.qll:1:1:4:1 | ClasslessPredicate foo | +| MultiResolve.qll:14:25:14:35 | PredicateCall | MultiResolve.qll:12:1:12:24 | ClasslessPredicate myFoo | +| MultiResolve.qll:25:7:25:18 | PredicateCall | MultiResolve.qll:17:3:19:3 | ClasslessPredicate bar | | Overrides.qll:8:30:8:39 | MemberCall | Overrides.qll:6:3:6:29 | ClassPredicate bar | | Overrides.qll:16:39:16:48 | MemberCall | Overrides.qll:6:3:6:29 | ClassPredicate bar | | Overrides.qll:16:39:16:48 | MemberCall | Overrides.qll:14:12:14:43 | ClassPredicate bar | @@ -43,5 +46,7 @@ exprPredicate | Foo.qll:26:22:26:31 | predicate | Foo.qll:20:3:20:54 | ClasslessPredicate myThing2 | | Foo.qll:47:55:47:62 | predicate | Foo.qll:42:20:42:27 | NewTypeBranch MkRoot | | Foo.qll:47:65:47:70 | predicate | Foo.qll:44:9:44:56 | ClasslessPredicate edge | +| MultiResolve.qll:12:19:12:23 | predicate | MultiResolve.qll:1:1:4:1 | ClasslessPredicate foo | +| MultiResolve.qll:12:19:12:23 | predicate | MultiResolve.qll:6:1:10:1 | ClasslessPredicate foo | | ParamModules.qll:4:18:4:25 | predicate | ParamModules.qll:2:13:2:36 | ClasslessPredicate fooSig | | ParamModules.qll:10:34:10:40 | predicate | ParamModules.qll:8:3:8:35 | ClasslessPredicate myFoo | From 115110475d3ba345a21b38c8b8891cba5835730d Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Sun, 19 Jun 2022 20:09:01 +0200 Subject: [PATCH 09/14] fix `getName()` on module instantiations --- ql/ql/src/codeql_ql/ast/Ast.qll | 4 ++-- ql/ql/src/codeql_ql/ast/internal/Module.qll | 16 ++++++++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/ql/ql/src/codeql_ql/ast/Ast.qll b/ql/ql/src/codeql_ql/ast/Ast.qll index a237e1f6b58..3a0063aaa76 100644 --- a/ql/ql/src/codeql_ql/ast/Ast.qll +++ b/ql/ql/src/codeql_ql/ast/Ast.qll @@ -2226,7 +2226,7 @@ class ModuleExpr extends TModuleExpr, TypeRef { or not exists(me.getName()) and result = me.getChild().(QL::SimpleId).getValue() or - result = me.getChild().(QL::ModuleInstantiation).getName().getChild().getValue() + result = me.getAFieldOrChild().(QL::ModuleInstantiation).getName().getChild().getValue() } /** @@ -2261,7 +2261,7 @@ class ModuleExpr extends TModuleExpr, TypeRef { * The result is either a `PredicateExpr` or a `TypeExpr`. */ SignatureExpr getArgument(int i) { - result.toQL() = me.getChild().(QL::ModuleInstantiation).getChild(i) + result.toQL() = me.getAFieldOrChild().(QL::ModuleInstantiation).getChild(i) } } diff --git a/ql/ql/src/codeql_ql/ast/internal/Module.qll b/ql/ql/src/codeql_ql/ast/internal/Module.qll index b487c98cc7e..d84f0b8122e 100644 --- a/ql/ql/src/codeql_ql/ast/internal/Module.qll +++ b/ql/ql/src/codeql_ql/ast/internal/Module.qll @@ -332,7 +332,19 @@ module ModConsistency { * } */ - query predicate noName(Module mod) { not exists(mod.getName()) } + query predicate noName(AstNode mod) { + mod instanceof Module and + not exists(mod.(Module).getName()) + or + mod instanceof ModuleExpr and + not exists(mod.(ModuleExpr).getName()) + } - query predicate nonUniqueName(Module mod) { count(mod.getName()) >= 2 } + query predicate nonUniqueName(AstNode mod) { + mod instanceof Module and + count(mod.(Module).getName()) >= 2 + or + mod instanceof ModuleExpr and + count(mod.(ModuleExpr).getName()) >= 2 + } } From f08f02ed66724fe55482746e5ade7f826ad64ddd Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Sun, 19 Jun 2022 20:38:16 +0200 Subject: [PATCH 10/14] use the explicit super type to resolve calls --- ql/ql/src/codeql_ql/ast/internal/Predicate.qll | 17 +++++++++++++---- ql/ql/test/callgraph/MultiResolve.qll | 16 ++++++++++++++++ ql/ql/test/callgraph/callgraph.expected | 3 ++- 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/ql/ql/src/codeql_ql/ast/internal/Predicate.qll b/ql/ql/src/codeql_ql/ast/internal/Predicate.qll index b4a5eaa6574..60b52c3241f 100644 --- a/ql/ql/src/codeql_ql/ast/internal/Predicate.qll +++ b/ql/ql/src/codeql_ql/ast/internal/Predicate.qll @@ -95,15 +95,24 @@ private module Cached { ) or // super calls - and `this.method()` calls in charpreds. (Basically: in charpreds there is no difference between super and this.) - exists(AstNode sup, ClassType type, Type supertype | + exists(AstNode sup, Type supertype | sup instanceof Super or sup.(ThisAccess).getEnclosingPredicate() instanceof CharPred | mc.getBase() = sup and - sup.getEnclosingPredicate().getParent().(Class).getType() = type and - supertype in [type.getASuperType(), type.getAnInstanceofType()] and - p = supertype.getClassPredicate(mc.getMemberName(), mc.getNumberOfArguments()) + p = supertype.getClassPredicate(mc.getMemberName(), mc.getNumberOfArguments()) and + ( + // super.method() + not exists(mc.getSuperType()) and + exists(ClassType type | + sup.getEnclosingPredicate().getParent().(Class).getType() = type and + supertype in [type.getASuperType(), type.getAnInstanceofType()] + ) + or + // Class.super.method() + supertype = mc.getSuperType().getResolvedType() + ) ) } diff --git a/ql/ql/test/callgraph/MultiResolve.qll b/ql/ql/test/callgraph/MultiResolve.qll index 161d86077ad..aabb680709d 100644 --- a/ql/ql/test/callgraph/MultiResolve.qll +++ b/ql/ql/test/callgraph/MultiResolve.qll @@ -24,3 +24,19 @@ module MyMod { } } } + +class Super1 extends int { + Super1() { this = 42 } + + predicate foo() { any() } +} + +class Super2 extends int { + Super2() { this = 42 } + + predicate foo() { none() } +} + +class Sub extends Super1, Super2 { + override predicate foo() { Super1.super.foo() } // <- should resolve to Super1::foo() +} diff --git a/ql/ql/test/callgraph/callgraph.expected b/ql/ql/test/callgraph/callgraph.expected index b640afaecc6..52cd12bcb53 100644 --- a/ql/ql/test/callgraph/callgraph.expected +++ b/ql/ql/test/callgraph/callgraph.expected @@ -16,7 +16,8 @@ getTarget | Foo.qll:38:39:38:67 | MemberCall | file://:0:0:0:0 | regexpCapture | | MultiResolve.qll:14:25:14:35 | PredicateCall | MultiResolve.qll:1:1:4:1 | ClasslessPredicate foo | | MultiResolve.qll:14:25:14:35 | PredicateCall | MultiResolve.qll:12:1:12:24 | ClasslessPredicate myFoo | -| MultiResolve.qll:25:7:25:18 | PredicateCall | MultiResolve.qll:17:3:19:3 | ClasslessPredicate bar | +| MultiResolve.qll:23:7:23:18 | PredicateCall | MultiResolve.qll:17:3:17:27 | ClasslessPredicate bar | +| MultiResolve.qll:41:30:41:47 | MemberCall | MultiResolve.qll:31:3:31:27 | ClassPredicate foo | | Overrides.qll:8:30:8:39 | MemberCall | Overrides.qll:6:3:6:29 | ClassPredicate bar | | Overrides.qll:16:39:16:48 | MemberCall | Overrides.qll:6:3:6:29 | ClassPredicate bar | | Overrides.qll:16:39:16:48 | MemberCall | Overrides.qll:14:12:14:43 | ClassPredicate bar | From f8b451a514bcd55db4a5c3ae645171744fb64bdc Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Sun, 19 Jun 2022 22:38:09 +0200 Subject: [PATCH 11/14] get all calls to resolve to a unique predicate (within reason) --- ql/ql/src/codeql_ql/ast/internal/Module.qll | 3 +- .../src/codeql_ql/ast/internal/Predicate.qll | 46 +++++++++++++------ .../queries/diagnostics/EmptyConsistencies.ql | 2 + ql/ql/test/callgraph/MultiResolve.qll | 16 +++++++ ql/ql/test/callgraph/callgraph.expected | 5 +- 5 files changed, 54 insertions(+), 18 deletions(-) diff --git a/ql/ql/src/codeql_ql/ast/internal/Module.qll b/ql/ql/src/codeql_ql/ast/internal/Module.qll index d84f0b8122e..4b224ad4adf 100644 --- a/ql/ql/src/codeql_ql/ast/internal/Module.qll +++ b/ql/ql/src/codeql_ql/ast/internal/Module.qll @@ -229,7 +229,8 @@ private module Cached { pragma[noinline] private predicate resolveModuleRefHelper(TypeRef me, ContainerOrModule enclosing, string name) { enclosing = getEnclosingModule(me).getEnclosing*() and - name = [me.(ModuleExpr).getName(), me.(TypeExpr).getClassName()] + name = [me.(ModuleExpr).getName(), me.(TypeExpr).getClassName()] and + (not me instanceof ModuleExpr or not enclosing instanceof Folder_) // module expressions are not imports, so they can't resolve to a file (which is contained in a folder). } } diff --git a/ql/ql/src/codeql_ql/ast/internal/Predicate.qll b/ql/ql/src/codeql_ql/ast/internal/Predicate.qll index 60b52c3241f..e2da87b1904 100644 --- a/ql/ql/src/codeql_ql/ast/internal/Predicate.qll +++ b/ql/ql/src/codeql_ql/ast/internal/Predicate.qll @@ -88,7 +88,23 @@ private module Cached { ) } + /** + * Holds if `mc` is a `this.method()` call to a predicate defined in the same class. + * helps avoid spuriously resolving to predicates in super-classes. + */ + private predicate resolveSelfClassCalls(MemberCall mc, PredicateOrBuiltin p) { + exists(Class c | + mc.getBase() instanceof ThisAccess and + c = mc.getEnclosingPredicate().getParent() and + p = c.getClassPredicate(mc.getMemberName()) and + p.getArity() = mc.getNumberOfArguments() + ) + } + private predicate resolveMemberCall(MemberCall mc, PredicateOrBuiltin p) { + resolveSelfClassCalls(mc, p) + or + not resolveSelfClassCalls(mc, _) and exists(Type t | t = mc.getBase().getType() and p = t.getClassPredicate(mc.getMemberName(), mc.getNumberOfArguments()) @@ -188,20 +204,20 @@ module PredConsistency { c > 1 and resolvePredicateExpr(pe, p) } - // This can happen with parameterized modules - /* - * query predicate multipleResolveCall(Call call, int c, PredicateOrBuiltin p) { - * c = - * strictcount(PredicateOrBuiltin p0 | - * resolveCall(call, p0) and - * // aliases are expected to resolve to multiple. - * not exists(p0.(ClasslessPredicate).getAlias()) and - * // overridden predicates may have multiple targets - * not p0.(ClassPredicate).isOverride() - * ) and - * c > 1 and - * resolveCall(call, p) - * } - */ + query predicate multipleResolveCall(Call call, int c, PredicateOrBuiltin p) { + c = + strictcount(PredicateOrBuiltin p0 | + resolveCall(call, p0) and + // aliases are expected to resolve to multiple. + not exists(p0.(ClasslessPredicate).getAlias()) and + // overridden predicates may have multiple targets + not p0.(ClassPredicate).isOverride() and + not p0 instanceof Relation // <- DB relations resolve to both a relation and a predicate. + ) and + c > 1 and + resolveCall(call, p) and + // parameterized modules are expected to resolve to multiple. + not exists(Predicate sig | not exists(sig.getBody()) and resolveCall(call, sig)) } +} diff --git a/ql/ql/src/queries/diagnostics/EmptyConsistencies.ql b/ql/ql/src/queries/diagnostics/EmptyConsistencies.ql index 7f95fec935e..8cd89f7fd37 100644 --- a/ql/ql/src/queries/diagnostics/EmptyConsistencies.ql +++ b/ql/ql/src/queries/diagnostics/EmptyConsistencies.ql @@ -22,6 +22,8 @@ where or PredConsistency::noResolvePredicateExpr(node) and msg = "PredConsistency::noResolvePredicateExpr" or + PredConsistency::multipleResolveCall(node, _, _) and msg = "PredConsistency::multipleResolveCall" + or TypeConsistency::exprNoType(node) and msg = "TypeConsistency::exprNoType" or TypeConsistency::varDefNoType(node) and msg = "TypeConsistency::varDefNoType" diff --git a/ql/ql/test/callgraph/MultiResolve.qll b/ql/ql/test/callgraph/MultiResolve.qll index aabb680709d..dce88d01689 100644 --- a/ql/ql/test/callgraph/MultiResolve.qll +++ b/ql/ql/test/callgraph/MultiResolve.qll @@ -40,3 +40,19 @@ class Super2 extends int { class Sub extends Super1, Super2 { override predicate foo() { Super1.super.foo() } // <- should resolve to Super1::foo() } + +module Foo { + predicate foo() { any() } +} + +predicate test() { + Foo::foo() // <- should resolve to `foo` from the module above, and not from the `Foo.qll` file. +} + +class Sub2 extends Super1, Super2 { + override predicate foo() { Super2.super.foo() } // <- should resolve to Super2::foo() + + predicate test() { + this.foo() // <- should resolve to only the above `foo` predicate, but currently it resolves to that, and all the overrides [INCONSISTENCY] + } +} diff --git a/ql/ql/test/callgraph/callgraph.expected b/ql/ql/test/callgraph/callgraph.expected index 52cd12bcb53..4d7840383cf 100644 --- a/ql/ql/test/callgraph/callgraph.expected +++ b/ql/ql/test/callgraph/callgraph.expected @@ -18,10 +18,11 @@ getTarget | MultiResolve.qll:14:25:14:35 | PredicateCall | MultiResolve.qll:12:1:12:24 | ClasslessPredicate myFoo | | MultiResolve.qll:23:7:23:18 | PredicateCall | MultiResolve.qll:17:3:17:27 | ClasslessPredicate bar | | MultiResolve.qll:41:30:41:47 | MemberCall | MultiResolve.qll:31:3:31:27 | ClassPredicate foo | +| MultiResolve.qll:49:3:49:12 | PredicateCall | MultiResolve.qll:45:3:45:27 | ClasslessPredicate foo | +| MultiResolve.qll:53:30:53:47 | MemberCall | MultiResolve.qll:37:3:37:28 | ClassPredicate foo | +| MultiResolve.qll:56:5:56:14 | MemberCall | MultiResolve.qll:53:12:53:49 | ClassPredicate foo | | Overrides.qll:8:30:8:39 | MemberCall | Overrides.qll:6:3:6:29 | ClassPredicate bar | -| Overrides.qll:16:39:16:48 | MemberCall | Overrides.qll:6:3:6:29 | ClassPredicate bar | | Overrides.qll:16:39:16:48 | MemberCall | Overrides.qll:14:12:14:43 | ClassPredicate bar | -| Overrides.qll:24:39:24:48 | MemberCall | Overrides.qll:6:3:6:29 | ClassPredicate bar | | Overrides.qll:24:39:24:48 | MemberCall | Overrides.qll:22:12:22:44 | ClassPredicate bar | | Overrides.qll:28:3:28:9 | MemberCall | Overrides.qll:6:3:6:29 | ClassPredicate bar | | Overrides.qll:29:3:29:10 | MemberCall | Overrides.qll:8:3:8:41 | ClassPredicate baz | From 15f9e084d5962b1d3b5930d935574a8d2c8c7d77 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Sun, 19 Jun 2022 22:43:41 +0200 Subject: [PATCH 12/14] fix spurious resolved predicate expressions --- ql/ql/src/codeql_ql/ast/internal/Predicate.qll | 10 ++++++++-- ql/ql/src/queries/diagnostics/EmptyConsistencies.ql | 3 +++ ql/ql/test/callgraph/callgraph.expected | 1 - 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/ql/ql/src/codeql_ql/ast/internal/Predicate.qll b/ql/ql/src/codeql_ql/ast/internal/Predicate.qll index e2da87b1904..6b17216b9aa 100644 --- a/ql/ql/src/codeql_ql/ast/internal/Predicate.qll +++ b/ql/ql/src/codeql_ql/ast/internal/Predicate.qll @@ -49,7 +49,8 @@ private module Cached { m = pe.getQualifier().getResolvedModule() and public = true | - definesPredicate(m, pe.getName(), p.getArity(), p, public) + definesPredicate(m, pe.getName(), p.getArity(), p, public) and + p.getArity() = pe.getArity() ) } @@ -200,7 +201,12 @@ module PredConsistency { } query predicate multipleResolvePredicateExpr(PredicateExpr pe, int c, ClasslessPredicate p) { - c = strictcount(ClasslessPredicate p0 | resolvePredicateExpr(pe, p0)) and + c = + strictcount(ClasslessPredicate p0 | + resolvePredicateExpr(pe, p0) and + // aliases are expected to resolve to multiple. + not exists(p0.(ClasslessPredicate).getAlias()) + ) and c > 1 and resolvePredicateExpr(pe, p) } diff --git a/ql/ql/src/queries/diagnostics/EmptyConsistencies.ql b/ql/ql/src/queries/diagnostics/EmptyConsistencies.ql index 8cd89f7fd37..8691aa2168c 100644 --- a/ql/ql/src/queries/diagnostics/EmptyConsistencies.ql +++ b/ql/ql/src/queries/diagnostics/EmptyConsistencies.ql @@ -24,6 +24,9 @@ where or PredConsistency::multipleResolveCall(node, _, _) and msg = "PredConsistency::multipleResolveCall" or + PredConsistency::multipleResolvePredicateExpr(node, _, _) and + msg = "PredConsistency::multipleResolvePredicateExpr" + or TypeConsistency::exprNoType(node) and msg = "TypeConsistency::exprNoType" or TypeConsistency::varDefNoType(node) and msg = "TypeConsistency::varDefNoType" diff --git a/ql/ql/test/callgraph/callgraph.expected b/ql/ql/test/callgraph/callgraph.expected index 4d7840383cf..de11b0d658c 100644 --- a/ql/ql/test/callgraph/callgraph.expected +++ b/ql/ql/test/callgraph/callgraph.expected @@ -49,6 +49,5 @@ exprPredicate | Foo.qll:47:55:47:62 | predicate | Foo.qll:42:20:42:27 | NewTypeBranch MkRoot | | Foo.qll:47:65:47:70 | predicate | Foo.qll:44:9:44:56 | ClasslessPredicate edge | | MultiResolve.qll:12:19:12:23 | predicate | MultiResolve.qll:1:1:4:1 | ClasslessPredicate foo | -| MultiResolve.qll:12:19:12:23 | predicate | MultiResolve.qll:6:1:10:1 | ClasslessPredicate foo | | ParamModules.qll:4:18:4:25 | predicate | ParamModules.qll:2:13:2:36 | ClasslessPredicate fooSig | | ParamModules.qll:10:34:10:40 | predicate | ParamModules.qll:8:3:8:35 | ClasslessPredicate myFoo | From 6d3808bd899210863868c42b4f41369481bbd73f Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Sun, 19 Jun 2022 23:19:01 +0200 Subject: [PATCH 13/14] remove redundant cast --- ql/ql/src/codeql_ql/ast/internal/Predicate.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/ql/src/codeql_ql/ast/internal/Predicate.qll b/ql/ql/src/codeql_ql/ast/internal/Predicate.qll index 6b17216b9aa..d419534788b 100644 --- a/ql/ql/src/codeql_ql/ast/internal/Predicate.qll +++ b/ql/ql/src/codeql_ql/ast/internal/Predicate.qll @@ -205,7 +205,7 @@ module PredConsistency { strictcount(ClasslessPredicate p0 | resolvePredicateExpr(pe, p0) and // aliases are expected to resolve to multiple. - not exists(p0.(ClasslessPredicate).getAlias()) + not exists(p0.getAlias()) ) and c > 1 and resolvePredicateExpr(pe, p) From 7d62b9e13179c8712ad9aa0994de24fd75442e94 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 20 Jun 2022 12:12:57 +0200 Subject: [PATCH 14/14] move the pruning for module resolution of TypeExprs --- ql/ql/src/codeql_ql/ast/Ast.qll | 11 +---------- ql/ql/src/codeql_ql/ast/internal/Module.qll | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/ql/ql/src/codeql_ql/ast/Ast.qll b/ql/ql/src/codeql_ql/ast/Ast.qll index 3a0063aaa76..c411f287fbc 100644 --- a/ql/ql/src/codeql_ql/ast/Ast.qll +++ b/ql/ql/src/codeql_ql/ast/Ast.qll @@ -683,16 +683,7 @@ class TypeExpr extends TType, TypeRef { resolveTypeExpr(this, result) or // if it resolves to a module, - exists(FileOrModule mod | resolveModuleRef(this, mod) | result = mod.toType()) and - result instanceof ModuleType and - // we can get spurious results in some cases, so we restrict to where it is possible to have a module. - ( - // only possible if this is inside a moduleInstantiation. - this = any(ModuleExpr mod).getArgument(_).asType() - or - // or if it's a parameter to a parameterized module - this = any(SignatureExpr sig, Module mod | mod.hasParameter(_, _, sig) | sig).asType() - ) + exists(FileOrModule mod | resolveModuleRef(this, mod) | result = mod.toType()) } override AstNode getAChild(string pred) { diff --git a/ql/ql/src/codeql_ql/ast/internal/Module.qll b/ql/ql/src/codeql_ql/ast/internal/Module.qll index 4b224ad4adf..1ca459ac108 100644 --- a/ql/ql/src/codeql_ql/ast/internal/Module.qll +++ b/ql/ql/src/codeql_ql/ast/internal/Module.qll @@ -218,6 +218,20 @@ private module Cached { not exists(me.(ModuleExpr).getQualifier()) and exists(ContainerOrModule enclosing, string name | resolveModuleRefHelper(me, enclosing, name) | definesModule(enclosing, name, m, _) + ) and + ( + not me instanceof TypeExpr + or + // remove some spurious results that can happen with `TypeExpr` + me instanceof TypeExpr and + m instanceof Module_ and // TypeExpr can only resolve to a Module, and only in some scenarios + ( + // only possible if this is inside a moduleInstantiation. + me = any(ModuleExpr mod).getArgument(_).asType() + or + // or if it's a parameter to a parameterized module + me = any(SignatureExpr sig, Module mod | mod.hasParameter(_, _, sig) | sig).asType() + ) ) or exists(FileOrModule mid |