Merge branch 'main' into sql1

Author: geoffw0

.github/workflows/codeql-analysis.yml

@@ -30,7 +30,7 @@ jobs:
     - name: Setup dotnet
       uses: actions/setup-dotnet@v4
       with:
-        dotnet-version: 8.0.101
+        dotnet-version: 9.0.100
 
     - name: Checkout repository
       uses: actions/checkout@v4

.github/workflows/cpp-swift-analysis.yml

@@ -48,7 +48,7 @@ jobs:
     - name: "Build Swift extractor using Bazel"
       run: |
         bazel clean --expunge
-        bazel run //swift:create-extractor-pack --nouse_action_cache --noremote_accept_cached --noremote_upload_local_results --spawn_strategy=local
+        bazel run //swift:install --nouse_action_cache --noremote_accept_cached --noremote_upload_local_results --spawn_strategy=local
         bazel shutdown
 
     - name: Perform CodeQL Analysis

.github/workflows/csharp-qltest.yml

@@ -39,14 +39,14 @@ jobs:
       - name: Setup dotnet
         uses: actions/setup-dotnet@v4
         with:
-          dotnet-version: 8.0.101
+          dotnet-version: 9.0.100
       - name: Extractor unit tests
         run: |
           dotnet tool restore
-          dotnet test -p:RuntimeFrameworkVersion=8.0.1 extractor/Semmle.Util.Tests
-          dotnet test -p:RuntimeFrameworkVersion=8.0.1 extractor/Semmle.Extraction.Tests
-          dotnet test -p:RuntimeFrameworkVersion=8.0.1 autobuilder/Semmle.Autobuild.CSharp.Tests
-          dotnet test -p:RuntimeFrameworkVersion=8.0.1 autobuilder/Semmle.Autobuild.Cpp.Tests
+          dotnet test -p:RuntimeFrameworkVersion=9.0.0 extractor/Semmle.Util.Tests
+          dotnet test -p:RuntimeFrameworkVersion=9.0.0 extractor/Semmle.Extraction.Tests
+          dotnet test -p:RuntimeFrameworkVersion=9.0.0 autobuilder/Semmle.Autobuild.CSharp.Tests
+          dotnet test -p:RuntimeFrameworkVersion=9.0.0 autobuilder/Semmle.Autobuild.Cpp.Tests
         shell: bash
   stubgentest:
     runs-on: ubuntu-latest

MODULE.bazel

@@ -25,7 +25,7 @@ bazel_dep(name = "nlohmann_json", version = "3.11.3", repo_name = "json")
 bazel_dep(name = "fmt", version = "10.0.0")
 bazel_dep(name = "rules_kotlin", version = "2.0.0-codeql.1")
 bazel_dep(name = "gazelle", version = "0.38.0")
-bazel_dep(name = "rules_dotnet", version = "0.16.1")
+bazel_dep(name = "rules_dotnet", version = "0.17.4")
 bazel_dep(name = "googletest", version = "1.14.0.bcr.1")
 bazel_dep(name = "rules_rust", version = "0.52.2")
 
@@ -71,7 +71,7 @@ tree_sitter_extractors_deps = use_extension("//misc/bazel/3rdparty:tree_sitter_e
 use_repo(tree_sitter_extractors_deps, "vendor__anyhow-1.0.93", "vendor__argfile-0.2.1", "vendor__chrono-0.4.38", "vendor__clap-4.5.20", "vendor__encoding-0.2.33", "vendor__figment-0.10.19", "vendor__flate2-1.0.34", "vendor__glob-0.3.1", "vendor__globset-0.4.15", "vendor__itertools-0.10.5", "vendor__itertools-0.13.0", "vendor__lazy_static-1.5.0", "vendor__log-0.4.22", "vendor__num-traits-0.2.19", "vendor__num_cpus-1.16.0", "vendor__proc-macro2-1.0.89", "vendor__quote-1.0.37", "vendor__ra_ap_base_db-0.0.232", "vendor__ra_ap_cfg-0.0.232", "vendor__ra_ap_hir-0.0.232", "vendor__ra_ap_hir_def-0.0.232", "vendor__ra_ap_hir_expand-0.0.232", "vendor__ra_ap_ide_db-0.0.232", "vendor__ra_ap_intern-0.0.232", "vendor__ra_ap_load-cargo-0.0.232", "vendor__ra_ap_parser-0.0.232", "vendor__ra_ap_paths-0.0.232", "vendor__ra_ap_project_model-0.0.232", "vendor__ra_ap_span-0.0.232", "vendor__ra_ap_syntax-0.0.232", "vendor__ra_ap_vfs-0.0.232", "vendor__rand-0.8.5", "vendor__rayon-1.10.0", "vendor__regex-1.11.1", "vendor__serde-1.0.214", "vendor__serde_json-1.0.132", "vendor__serde_with-3.11.0", "vendor__stderrlog-0.6.0", "vendor__syn-2.0.87", "vendor__tracing-0.1.40", "vendor__tracing-subscriber-0.3.18", "vendor__tree-sitter-0.24.4", "vendor__tree-sitter-embedded-template-0.23.2", "vendor__tree-sitter-json-0.24.8", "vendor__tree-sitter-ql-0.23.1", "vendor__tree-sitter-ruby-0.23.1", "vendor__triomphe-0.1.14", "vendor__ungrammar-1.16.1")
 
 dotnet = use_extension("@rules_dotnet//dotnet:extensions.bzl", "dotnet")
-dotnet.toolchain(dotnet_version = "8.0.101")
+dotnet.toolchain(dotnet_version = "9.0.100")
 use_repo(dotnet, "dotnet_toolchains")
 
 register_toolchains("@dotnet_toolchains//:all")

actions/BUILD.bazel

@@ -2,19 +2,8 @@ load("//misc/bazel:pkg.bzl", "codeql_pack")
 
 package(default_visibility = ["//visibility:public"])
 
-[
-    codeql_pack(
-        name = "-".join(parts),
-        srcs = [
-            "//actions/extractor",
-        ],
-        pack_prefix = "/".join(parts),
-    )
-    for parts in (
-        [
-            "experimental",
-            "actions",
-        ],
-        ["actions"],
-    )
-]
+codeql_pack(
+    name = "actions",
+    srcs = ["//actions/extractor"],
+    experimental = True,
+)

cpp/ql/src/Critical/UseAfterFree.qhelp

@@ -8,7 +8,7 @@
 <p>
 This rule finds accesses through a pointer of a memory location that has already been freed (i.e. through a dangling pointer). 
 Such memory blocks have already been released to the dynamic memory manager, and modifying them can lead to anything 
-from a segfault to memory corruption that would cause subsequent calls to the dynamic memory manger to behave 
+from a segfault to memory corruption that would cause subsequent calls to the dynamic memory manager to behave 
 erratically, to a possible security vulnerability.
 </p>
 

cpp/ql/src/experimental/Best Practices/GuardedFree.ql

@@ -18,9 +18,31 @@ class FreeCall extends FunctionCall {
   FreeCall() { this.getTarget().hasGlobalName("free") }
 }
 
+predicate blockContainsPreprocessorBranches(BasicBlock bb) {
+  exists(PreprocessorBranch ppb, Location bbLoc, Location ppbLoc |
+    bbLoc = bb.(Stmt).getLocation() and ppbLoc = ppb.getLocation()
+  |
+    bbLoc.getFile() = ppb.getFile() and
+    bbLoc.getStartLine() < ppbLoc.getStartLine() and
+    ppbLoc.getEndLine() < bbLoc.getEndLine()
+  )
+}
+
 from GuardCondition gc, FreeCall fc, Variable v, BasicBlock bb
 where
   gc.ensuresEq(v.getAnAccess(), 0, bb, false) and
   fc.getArgument(0) = v.getAnAccess() and
-  bb = fc.getEnclosingStmt()
+  bb = fc.getBasicBlock() and
+  (
+    // No block statement: if (x) free(x);
+    bb = fc.getEnclosingStmt()
+    or
+    // Block statement with a single nested statement: if (x) { free(x); }
+    strictcount(bb.(BlockStmt).getAStmt()) = 1
+  ) and
+  strictcount(BasicBlock bb2 | gc.ensuresEq(_, 0, bb2, _) | bb2) = 1 and
+  not fc.isInMacroExpansion() and
+  not blockContainsPreprocessorBranches(bb) and
+  not (gc instanceof BinaryOperation and not gc instanceof ComparisonOperation) and
+  not exists(CommaExpr c | c.getAChild*() = fc)
 select gc, "unnecessary NULL check before call to $@", fc, "free"

cpp/ql/test/experimental/query-tests/Best Practices/GuardedFree/GuardedFree.expected

@@ -1,10 +1,5 @@
 | test.cpp:5:7:5:7 | x | unnecessary NULL check before call to $@ | test.cpp:6:5:6:8 | call to free | free |
-| test.cpp:23:7:23:7 | x | unnecessary NULL check before call to $@ | test.cpp:26:5:26:8 | call to free | free |
-| test.cpp:31:7:31:8 | ! ... | unnecessary NULL check before call to $@ | test.cpp:35:3:35:6 | call to free | free |
-| test.cpp:31:7:31:24 | ... \|\| ... | unnecessary NULL check before call to $@ | test.cpp:35:3:35:6 | call to free | free |
-| test.cpp:31:8:31:8 | x | unnecessary NULL check before call to $@ | test.cpp:35:3:35:6 | call to free | free |
-| test.cpp:94:12:94:12 | x | unnecessary NULL check before call to $@ | test.cpp:94:3:94:13 | call to free | free |
-| test.cpp:98:7:98:8 | ! ... | unnecessary NULL check before call to $@ | test.cpp:101:3:101:6 | call to free | free |
-| test.cpp:98:8:98:8 | x | unnecessary NULL check before call to $@ | test.cpp:101:3:101:6 | call to free | free |
+| test.cpp:10:7:10:7 | x | unnecessary NULL check before call to $@ | test.cpp:11:5:11:8 | call to free | free |
+| test.cpp:42:7:42:7 | x | unnecessary NULL check before call to $@ | test.cpp:43:5:43:8 | call to free | free |
+| test.cpp:49:7:49:7 | x | unnecessary NULL check before call to $@ | test.cpp:50:5:50:8 | call to free | free |
 | test.cpp:106:7:106:18 | ... != ... | unnecessary NULL check before call to $@ | test.cpp:107:5:107:8 | call to free | free |
-| test.cpp:113:7:113:18 | ... != ... | unnecessary NULL check before call to $@ | test.cpp:114:17:114:20 | call to free | free |

cpp/ql/test/experimental/query-tests/Best Practices/GuardedFree/test.cpp

@@ -20,15 +20,15 @@ void test2(int *x) {
 }
 
 void test3(int *x, bool b) {
-  if (x) { // GOOD [FALSE POSITIVE]: x is being accessed in the body of the if
+  if (x) { // GOOD: x is being accessed in the body of the if
     if (b)
       *x = 42;
     free(x);
   }
 }
 
 bool test4(char *x, char *y) {
-  if (!x || strcmp(x, y)) { // GOOD [FALSE POSITIVE]: x is being accessed in the guard and return value depends on x
+  if (!x || strcmp(x, y)) { // GOOD: x is being accessed in the guard and return value depends on x
     free(x);
     return true;
   }
@@ -91,11 +91,11 @@ void test10(char *x) {
   if (x) free(x);
 
 void test11(char *x) {
-  TRY_FREE(x) // BAD
+  TRY_FREE(x) // BAD [NOT DETECTED]
 }
 
 bool test12(char *x) {
-  if (!x) // GOOD [FALSE POSITIVE]: return value depends on x
+  if (!x) // GOOD: return value depends on x
     return false;
 
   free(x);
@@ -110,6 +110,6 @@ void test13(char *x) {
 void inspect(char *x);
 
 void test14(char *x) {
-  if (x != nullptr) // GOOD [FALSE POSITIVE]: x might be accessed in the first operand of the comma operator
+  if (x != nullptr) // GOOD: x might be accessed in the first operand of the comma operator
     inspect(x), free(x);
 }

csharp/.config/dotnet-tools.json

@@ -3,7 +3,7 @@
   "isRoot": true,
   "tools": {
     "paket": {
-      "version": "8.0.3",
+      "version": "9.0.1",
       "commands": [
         "paket"
       ]