Merge branch 'main' into use-range-analysis-in-buffer-write

Author: MathiasVP

.github/actions/fetch-codeql/action.yml

@@ -8,7 +8,7 @@ runs:
       run: |
         LATEST=$(gh release list --repo https://github.com/github/codeql-cli-binaries | cut -f 1 | grep -v beta | sort --version-sort | tail -1)
         gh release download --repo https://github.com/github/codeql-cli-binaries --pattern codeql-linux64.zip "$LATEST"
-        unzip -q codeql-linux64.zip
-        echo "${{ github.workspace }}/codeql" >> $GITHUB_PATH
+        unzip -q -d "${RUNNER_TEMP}" codeql-linux64.zip
+        echo "${RUNNER_TEMP}/codeql" >> "${GITHUB_PATH}"
       env:
         GITHUB_TOKEN: ${{ github.token }}

.github/workflows/post-pr-comment.yml

@@ -0,0 +1,31 @@
+name: Post pull-request comment
+on:
+  workflow_run:
+    workflows: ["Query help preview"]
+    types:
+      - completed
+
+permissions:
+  pull-requests: write
+
+jobs:
+  post_comment:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Download artifact
+        run: gh run download "${WORKFLOW_RUN_ID}" --repo "${GITHUB_REPOSITORY}" --name "comment"
+        env:
+          GITHUB_TOKEN: ${{ github.token }}
+          WORKFLOW_RUN_ID: ${{ github.event.workflow_run.id }}
+      - run: |
+          PR="$(grep -o '^[0-9]\+$' pr.txt)"
+          PR_HEAD_SHA="$(gh api "/repos/${GITHUB_REPOSITORY}/pulls/${PR}" --jq .head.sha)"
+          # Check that the pull-request head SHA matches the head SHA of the workflow run
+          if [ "${WORKFLOW_RUN_HEAD_SHA}" != "${PR_HEAD_SHA}" ]; then
+            echo "PR head SHA ${PR_HEAD_SHA} does not match workflow_run event SHA ${WORKFLOW_RUN_HEAD_SHA}. Stopping." 1>&2
+            exit 1
+          fi
+          gh pr comment "${PR}" --repo "${GITHUB_REPOSITORY}" -F comment.txt
+        env:
+          GITHUB_TOKEN: ${{ github.token }}
+          WORKFLOW_RUN_HEAD_SHA: ${{ github.event.workflow_run.head_commit.id }}

.github/workflows/qhelp-pr-preview.yml

@@ -1,39 +1,63 @@
 name: Query help preview
 
+permissions:
+  contents: read
+
 on:
   pull_request:
     branches:
       - main
-      - 'rc/*'
+      - "rc/*"
     paths:
       - "ruby/**/*.qhelp"
 
 jobs:
   qhelp:
     runs-on: ubuntu-latest
     steps:
+      - run: echo "${{  github.event.number }}" > pr.txt
+      - uses: actions/upload-artifact@v2
+        with:
+          name: comment
+          path: pr.txt
+          retention-days: 1
       - uses: actions/checkout@v2
         with:
           fetch-depth: 2
+          persist-credentials: false
+      - uses: ./.github/actions/fetch-codeql
       - name: Determine changed files
         id: changes
         run: |
-          echo -n "::set-output name=qhelp_files::"
-          (git diff --name-only --diff-filter=ACMRT HEAD~1 HEAD | grep .qhelp$ | grep -v .inc.qhelp;
-           git diff --name-only --diff-filter=ACMRT HEAD~1 HEAD | grep .inc.qhelp$ | xargs -d '\n' -rn1 basename | xargs -d '\n' -rn1 git grep -l) |
-           sort -u | xargs -d '\n' -n1 printf "'%s' "
-
-      - uses: ./.github/actions/fetch-codeql
+          (git diff -z --name-only --diff-filter=ACMRT HEAD~1 HEAD | grep -z '.qhelp$' | grep -z -v '.inc.qhelp';
+           git diff -z --name-only --diff-filter=ACMRT HEAD~1 HEAD | grep -z '.inc.qhelp$' | xargs --null -rn1 basename | xargs --null -rn1 git grep -z -l) |
+           grep -z '.qhelp$' | grep -z -v '^-' | sort -z -u > "${RUNNER_TEMP}/paths.txt"
 
       - name: QHelp preview
-        if: ${{ steps.changes.outputs.qhelp_files }}
         run: |
-          ( echo "QHelp previews:";
-          for path in ${{ steps.changes.outputs.qhelp_files }} ; do
+          EXIT_CODE=0
+          echo "QHelp previews:" > comment.txt
+          while read -r -d $'\0' path; do
+            if [ ! -f "${path}" ]; then
+               exit 1
+            fi
             echo "<details> <summary>${path}</summary>"
             echo
-            codeql generate query-help --format=markdown ${path}
+            codeql generate query-help --format=markdown -- "./${path}" 2> errors.txt || EXIT_CODE="$?"
+            if [ -s errors.txt ]; then
+               echo "# errors/warnings:"
+               echo '```'
+               cat errors.txt
+               cat errors.txt 1>&2
+               echo '```'
+            fi
             echo "</details>"
-          done) | gh pr comment "${{ github.event.pull_request.number }}" -F -
-        env:
-          GITHUB_TOKEN: ${{ github.token }}
+          done < "${RUNNER_TEMP}/paths.txt" >> comment.txt
+          exit "${EXIT_CODE}"
+
+      - if: always()
+        uses: actions/upload-artifact@v2
+        with:
+          name: comment
+          path: comment.txt
+          retention-days: 1

.github/workflows/ruby-build.yml

@@ -3,16 +3,18 @@ name: "Ruby: Build"
 on:
   push:
     paths:
-      - 'ruby/**'
+      - "ruby/**"
+      - .github/workflows/ruby-build.yml
     branches:
       - main
-      - 'rc/*'
+      - "rc/*"
   pull_request:
     paths:
-      - 'ruby/**'
+      - "ruby/**"
+      - .github/workflows/ruby-build.yml
     branches:
       - main
-      - 'rc/*'
+      - "rc/*"
   workflow_dispatch:
     inputs:
       tag:

.github/workflows/ruby-dataset-measure.yml

@@ -4,15 +4,17 @@ on:
   push:
     branches:
       - main
-      - 'rc/*'
+      - "rc/*"
     paths:
       - ruby/ql/lib/ruby.dbscheme
+      - .github/workflows/ruby-dataset-measure.yml
   pull_request:
     branches:
       - main
-      - 'rc/*'
+      - "rc/*"
     paths:
       - ruby/ql/lib/ruby.dbscheme
+      - .github/workflows/ruby-dataset-measure.yml
   workflow_dispatch:
 
 jobs:

.github/workflows/ruby-qltest.yml

@@ -3,16 +3,18 @@ name: "Ruby: Run QL Tests"
 on:
   push:
     paths:
-      - 'ruby/**'
+      - "ruby/**"
+      - .github/workflows/ruby-qltest.yml
     branches:
       - main
-      - 'rc/*'
+      - "rc/*"
   pull_request:
     paths:
-      - 'ruby/**'
+      - "ruby/**"
+      - .github/workflows/ruby-qltest.yml
     branches:
       - main
-      - 'rc/*'
+      - "rc/*"
 
 env:
   CARGO_TERM_COLOR: always
@@ -44,5 +46,5 @@ jobs:
         run: |
           echo >empty.trap
           codeql dataset import -S ql/lib/upgrades/initial/ruby.dbscheme testdb empty.trap
-          codeql dataset upgrade testdb --additional-packs ql/lib/upgrades
+          codeql dataset upgrade testdb --additional-packs ql/lib
           diff -q testdb/ruby.dbscheme ql/lib/ruby.dbscheme

cpp/ql/lib/semmle/code/cpp/commons/NullTermination.qll

@@ -3,11 +3,14 @@ private import semmle.code.cpp.models.interfaces.ArrayFunction
 private import semmle.code.cpp.models.implementations.Strcat
 import semmle.code.cpp.dataflow.DataFlow
 
-private predicate mayAddNullTerminatorHelper(Expr e, VariableAccess va, Expr e0) {
-  exists(StackVariable v0, Expr val |
-    exprDefinition(v0, e, val) and
-    val.getAChild*() = va and
-    mayAddNullTerminator(e0, v0.getAnAccess())
+/**
+ * Holds if the expression `e` assigns something including `va` to a
+ * stack variable `v0`.
+ */
+private predicate mayAddNullTerminatorHelper(Expr e, VariableAccess va, StackVariable v0) {
+  exists(Expr val |
+    exprDefinition(v0, e, val) and // `e` is `v0 := val`
+    val.getAChild*() = va
   )
 }
 
@@ -25,8 +28,8 @@ private predicate controlFlowNodeSuccessorTransitive(ControlFlowNode n1, Control
 }
 
 /**
- * Holds if the expression `e` may add a null terminator to the string in
- * variable `v`.
+ * Holds if the expression `e` may add a null terminator to the string
+ * accessed by `va`.
  */
 predicate mayAddNullTerminator(Expr e, VariableAccess va) {
   // Assignment: dereferencing or array access
@@ -43,8 +46,9 @@ predicate mayAddNullTerminator(Expr e, VariableAccess va) {
   )
   or
   // Assignment to another stack variable
-  exists(Expr e0 |
-    mayAddNullTerminatorHelper(pragma[only_bind_into](e), va, pragma[only_bind_into](e0)) and
+  exists(StackVariable v0, Expr e0 |
+    mayAddNullTerminatorHelper(e, va, v0) and
+    mayAddNullTerminator(pragma[only_bind_into](e0), pragma[only_bind_into](v0.getAnAccess())) and
     controlFlowNodeSuccessorTransitive(e, e0)
   )
   or

cpp/ql/lib/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -474,6 +474,24 @@ module TaintedWithPath {
     }
   }
 
+  /**
+   * INTERNAL: Do not use.
+   */
+  module Private {
+    /** Gets a predecessor `PathNode` of `pathNode`, if any. */
+    PathNode getAPredecessor(PathNode pathNode) { edges(result, pathNode) }
+
+    /** Gets the element that `pathNode` wraps, if any. */
+    Element getElementFromPathNode(PathNode pathNode) {
+      exists(DataFlow::Node node | node = pathNode.(WrapPathNode).inner().getNode() |
+        result = node.asExpr() or
+        result = node.asParameter()
+      )
+      or
+      result = pathNode.(EndpointPathNode).inner()
+    }
+  }
+
   private class WrapPathNode extends PathNode, TWrapPathNode {
     DataFlow3::PathNode inner() { this = TWrapPathNode(result) }
 

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/annotate_path_to_sink/dispatch.cpp

@@ -4,8 +4,8 @@ using SinkFunction = void (*)(int);
 
 void notSink(int notSinkParam);
 
-void callsSink(int sinkParam) {
-  sink(sinkParam); // $ ast,ir=31:28 ast,ir=32:31 ast,ir=34:22 MISSING: ast,ir=28
+void callsSink(int sinkParam) { // $ ir-path=31:28 ir-path=32:31 ir-path=34:22
+  sink(sinkParam); // $ ir-sink=31:28 ir-sink=32:31 ir-sink=34:22 ast=31:28 ast=32:31 ast=34:22 MISSING: ast,ir=28
 }
 
 struct {
@@ -25,13 +25,13 @@ void assignGlobals() {
 };
 
 void testStruct() {
-  globalStruct.sinkPtr(atoi(getenv("TAINTED"))); // $ MISSING: ast,ir
+  globalStruct.sinkPtr(atoi(getenv("TAINTED"))); // $ ir MISSING: ast
   globalStruct.notSinkPtr(atoi(getenv("TAINTED"))); // clean
 
-  globalUnion.sinkPtr(atoi(getenv("TAINTED"))); // $ ast,ir
-  globalUnion.notSinkPtr(atoi(getenv("TAINTED"))); // $ ast,ir
+  globalUnion.sinkPtr(atoi(getenv("TAINTED"))); // $ ast ir-path
+  globalUnion.notSinkPtr(atoi(getenv("TAINTED"))); // $ ast ir-path
 
-  globalSinkPtr(atoi(getenv("TAINTED"))); // $ ast,ir
+  globalSinkPtr(atoi(getenv("TAINTED"))); // $ ast ir-path
 }
 
 class B {
@@ -48,19 +48,19 @@ class D2 : public D1 {
 
 class D3 : public D2 {
     public:
-    void f(const char* p) override {
-        sink(p); // $ ast,ir=58:10 ast,ir=60:17 ast,ir=61:28 ast,ir=62:29 ast,ir=63:33 SPURIOUS: ast,ir=73:30
+    void f(const char* p) override { // $ ir-path=58:10 ir-path=60:17 ir-path=61:28 ir-path=62:29 ir-path=63:33 ir-path=73:30
+        sink(p); // $ ir-sink=58:10 ir-sink=60:17 ir-sink=61:28 ir-sink=62:29 ir-sink=63:33 ast=58:10 ast=60:17 ast=61:28 ast=62:29 ast=63:33 SPURIOUS: ast=73:30 ir-sink=73:30
     }
 };
 
 void test_dynamic_cast() {
     B* b = new D3();
-    b->f(getenv("VAR")); // $ ast,ir
+    b->f(getenv("VAR")); // $ ast ir-path
 
-    ((D2*)b)->f(getenv("VAR")); // $ ast,ir
-    static_cast<D2*>(b)->f(getenv("VAR")); // $ ast,ir
-    dynamic_cast<D2*>(b)->f(getenv("VAR")); // $ ast,ir
-    reinterpret_cast<D2*>(b)->f(getenv("VAR")); // $ ast,ir
+    ((D2*)b)->f(getenv("VAR")); // $ ast ir-path
+    static_cast<D2*>(b)->f(getenv("VAR")); // $ ast ir-path
+    dynamic_cast<D2*>(b)->f(getenv("VAR")); // $ ast ir-path
+    reinterpret_cast<D2*>(b)->f(getenv("VAR")); // $ ast ir-path
 
     B* b2 = new D2();
     b2->f(getenv("VAR"));
@@ -70,5 +70,5 @@ void test_dynamic_cast() {
     dynamic_cast<D2*>(b2)->f(getenv("VAR"));
     reinterpret_cast<D2*>(b2)->f(getenv("VAR"));
 
-    dynamic_cast<D3*>(b2)->f(getenv("VAR")); // $ SPURIOUS: ast,ir
+    dynamic_cast<D3*>(b2)->f(getenv("VAR")); // $ SPURIOUS: ast ir-path
 }

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/annotate_path_to_sink/tainted.ql

@@ -7,9 +7,10 @@ import cpp
 import semmle.code.cpp.security.TaintTrackingImpl as ASTTaintTracking
 import semmle.code.cpp.ir.dataflow.DefaultTaintTracking as IRDefaultTaintTracking
 import IRDefaultTaintTracking::TaintedWithPath as TaintedWithPath
+import TaintedWithPath::Private
 import TestUtilities.InlineExpectationsTest
 
-predicate isSink(Element sink) {
+predicate isSinkArgument(Element sink) {
   exists(FunctionCall call |
     call.getTarget().getName() = "sink" and
     sink = call.getAnArgument()
@@ -19,31 +20,34 @@ predicate isSink(Element sink) {
 predicate astTaint(Expr source, Element sink) { ASTTaintTracking::tainted(source, sink) }
 
 class SourceConfiguration extends TaintedWithPath::TaintTrackingConfiguration {
-  override predicate isSink(Element e) { any() }
+  override predicate isSink(Element e) { isSinkArgument(e) }
 }
 
-predicate irTaint(Expr source, Element sink) {
-  TaintedWithPath::taintedWithPath(source, sink, _, _)
+predicate irTaint(Element source, Element sink, string tag) {
+  exists(TaintedWithPath::PathNode sinkNode, TaintedWithPath::PathNode predNode |
+    TaintedWithPath::taintedWithPath(source, _, _, sinkNode) and
+    predNode = getAPredecessor*(sinkNode) and
+    sink = getElementFromPathNode(predNode) and
+    // Make sure the path is actually reachable from this predecessor.
+    // Otherwise, we could pick `predNode` to be b when `source` is
+    // `source1` in this dataflow graph:
+    // source1 ---> a ---> c ---> sinkNode
+    //                   ^
+    // source2 ---> b --/
+    source = getElementFromPathNode(getAPredecessor*(predNode)) and
+    if sinkNode = predNode then tag = "ir-sink" else tag = "ir-path"
+  )
 }
 
 class IRDefaultTaintTrackingTest extends InlineExpectationsTest {
   IRDefaultTaintTrackingTest() { this = "IRDefaultTaintTrackingTest" }
 
-  override string getARelevantTag() { result = "ir" }
+  override string getARelevantTag() { result = ["ir-path", "ir-sink"] }
 
   override predicate hasActualResult(Location location, string element, string tag, string value) {
-    exists(Expr source, Element tainted, int n |
-      tag = "ir" and
-      irTaint(source, tainted) and
-      (
-        isSink(tainted)
-        or
-        exists(Element sink |
-          isSink(sink) and
-          irTaint(tainted, sink)
-        )
-      ) and
-      n = strictcount(Expr otherSource | irTaint(otherSource, tainted)) and
+    exists(Element source, Element tainted, int n |
+      irTaint(source, tainted, tag) and
+      n = strictcount(Element otherSource | irTaint(otherSource, tainted, _)) and
       (
         n = 1 and value = ""
         or
@@ -70,10 +74,10 @@ class ASTTaintTrackingTest extends InlineExpectationsTest {
       tag = "ast" and
       astTaint(source, tainted) and
       (
-        isSink(tainted)
+        isSinkArgument(tainted)
         or
         exists(Element sink |
-          isSink(sink) and
+          isSinkArgument(sink) and
           astTaint(tainted, sink)
         )
       ) and