From fb15a4ea11859ddfc1ef5361b8729fed01137edd Mon Sep 17 00:00:00 2001 From: codeqlhelper <166422730+codeqlhelper@users.noreply.github.com> Date: Tue, 9 Apr 2024 00:03:05 +0800 Subject: [PATCH 1/9] Reduce the false alarms of `GlobalUseBeforeInit.ql` --- cpp/ql/src/Critical/GlobalUseBeforeInit.ql | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/Critical/GlobalUseBeforeInit.ql b/cpp/ql/src/Critical/GlobalUseBeforeInit.ql index 6c3435eeba91..080fa3a491da 100644 --- a/cpp/ql/src/Critical/GlobalUseBeforeInit.ql +++ b/cpp/ql/src/Critical/GlobalUseBeforeInit.ql @@ -98,10 +98,25 @@ predicate callReaches(Call call, ControlFlowNode successor) { ) } +// To avoid many false alarms like `static int a = 1;` +predicate initialisedAtDeclaration(GlobalVariable v) { + exists(VariableDeclarationEntry vde | + vde = v.getDefinition() + and vde.isDefinition() + ) +} + +// No need to initialize those variables +predicate isStdlibVariable(GlobalVariable v) { + v.getName() = ["stdin", "stdout", "stderr"] +} + from GlobalVariable v, Function f where uninitialisedBefore(v, f) and - useFunc(v, f) + useFunc(v, f) and + not initialisedAtDeclaration(v) and + not isStdlibVariable(v) select f, "The variable '" + v.getName() + " is used in this function but may not be initialized when it is called." From af2a7eadc39c4b717c07440ec2e06477ba23389b Mon Sep 17 00:00:00 2001 From: codeqlhelper <166422730+codeqlhelper@users.noreply.github.com> Date: Tue, 9 Apr 2024 00:07:40 +0800 Subject: [PATCH 2/9] Reduce false alarms of `InconsistentNullnessTesting.ql` We should ignore `checked` in a macro to avoid too many false alarms, --- cpp/ql/src/Critical/InconsistentNullnessTesting.ql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp/ql/src/Critical/InconsistentNullnessTesting.ql b/cpp/ql/src/Critical/InconsistentNullnessTesting.ql index dbe7f699d14d..cee3e65547b0 100644 --- a/cpp/ql/src/Critical/InconsistentNullnessTesting.ql +++ b/cpp/ql/src/Critical/InconsistentNullnessTesting.ql @@ -15,6 +15,8 @@ import cpp from StackVariable v, ControlFlowNode def, VariableAccess checked, VariableAccess unchecked where checked = v.getAnAccess() and + // The check can often be in a macro for handling exception + not checked.isInMacroExpansion() and dereferenced(checked) and unchecked = v.getAnAccess() and dereferenced(unchecked) and From fc26e148fd43a1872862dcc7c33ee5791ae5689d Mon Sep 17 00:00:00 2001 From: codeqlhelper <166422730+codeqlhelper@users.noreply.github.com> Date: Tue, 9 Apr 2024 02:00:52 +0800 Subject: [PATCH 3/9] Create 2024-04-09-reduce-FP.md --- cpp/ql/src/change-notes/released/2024-04-09-reduce-FP.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 cpp/ql/src/change-notes/released/2024-04-09-reduce-FP.md diff --git a/cpp/ql/src/change-notes/released/2024-04-09-reduce-FP.md b/cpp/ql/src/change-notes/released/2024-04-09-reduce-FP.md new file mode 100644 index 000000000000..0b9b3edbf43b --- /dev/null +++ b/cpp/ql/src/change-notes/released/2024-04-09-reduce-FP.md @@ -0,0 +1,6 @@ +--- +category: majorAnalysis +--- +* Reduce false positives of `GlobalUseBeforeInit.ql` and `InconsistentNullnessTesting.ql`. +Many global variables that are initialized at declaration will be reported by `GlobalUseBeforeInit.ql`. +When `checked` is in a macro expansion for handling exceptions, it is very likely for `InconsistentNullnessTesting.ql` to report false positives. From 334c0d0449777f6821d91ef98260bc61185ca95b Mon Sep 17 00:00:00 2001 From: "codeqlhelper@gmail.com" Date: Tue, 9 Apr 2024 02:06:06 +0800 Subject: [PATCH 4/9] ... --- cpp/ql/src/change-notes/{released => }/2024-04-09-reduce-FP.md | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename cpp/ql/src/change-notes/{released => }/2024-04-09-reduce-FP.md (100%) diff --git a/cpp/ql/src/change-notes/released/2024-04-09-reduce-FP.md b/cpp/ql/src/change-notes/2024-04-09-reduce-FP.md similarity index 100% rename from cpp/ql/src/change-notes/released/2024-04-09-reduce-FP.md rename to cpp/ql/src/change-notes/2024-04-09-reduce-FP.md From 8a92a4250fda7fce7c7d239dcfa573f1f95558d3 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 9 Apr 2024 17:53:33 +0100 Subject: [PATCH 5/9] C++: Autoformat. --- cpp/ql/src/Critical/GlobalUseBeforeInit.ql | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/cpp/ql/src/Critical/GlobalUseBeforeInit.ql b/cpp/ql/src/Critical/GlobalUseBeforeInit.ql index 080fa3a491da..7db8d25d8418 100644 --- a/cpp/ql/src/Critical/GlobalUseBeforeInit.ql +++ b/cpp/ql/src/Critical/GlobalUseBeforeInit.ql @@ -101,15 +101,13 @@ predicate callReaches(Call call, ControlFlowNode successor) { // To avoid many false alarms like `static int a = 1;` predicate initialisedAtDeclaration(GlobalVariable v) { exists(VariableDeclarationEntry vde | - vde = v.getDefinition() - and vde.isDefinition() + vde = v.getDefinition() and + vde.isDefinition() ) } // No need to initialize those variables -predicate isStdlibVariable(GlobalVariable v) { - v.getName() = ["stdin", "stdout", "stderr"] -} +predicate isStdlibVariable(GlobalVariable v) { v.getName() = ["stdin", "stdout", "stderr"] } from GlobalVariable v, Function f where From 132bb9f1d6cb630466cd4649404203fc80a857c2 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 16 Apr 2024 13:53:27 +0100 Subject: [PATCH 6/9] C++: Address (my own) review comments. --- cpp/ql/src/Critical/GlobalUseBeforeInit.ql | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/cpp/ql/src/Critical/GlobalUseBeforeInit.ql b/cpp/ql/src/Critical/GlobalUseBeforeInit.ql index 7db8d25d8418..6b286fbcca7f 100644 --- a/cpp/ql/src/Critical/GlobalUseBeforeInit.ql +++ b/cpp/ql/src/Critical/GlobalUseBeforeInit.ql @@ -98,16 +98,11 @@ predicate callReaches(Call call, ControlFlowNode successor) { ) } -// To avoid many false alarms like `static int a = 1;` -predicate initialisedAtDeclaration(GlobalVariable v) { - exists(VariableDeclarationEntry vde | - vde = v.getDefinition() and - vde.isDefinition() - ) -} +/** Holds if `v` has an initializer. */ +predicate initialisedAtDeclaration(GlobalVariable v) { exists(v.getInitializer()) } -// No need to initialize those variables -predicate isStdlibVariable(GlobalVariable v) { v.getName() = ["stdin", "stdout", "stderr"] } +/** Holds if `v` is a global variable that does not need to be initialized. */ +predicate isStdlibVariable(GlobalVariable v) { v.hasGlobalName(["stdin", "stdout", "stderr"]) } from GlobalVariable v, Function f where From 439afd97baff457b0e6624e1fc3ae5ba8f565cce Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 16 Apr 2024 13:54:58 +0100 Subject: [PATCH 7/9] C++: Small performance optimization. --- cpp/ql/src/Critical/GlobalUseBeforeInit.ql | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/Critical/GlobalUseBeforeInit.ql b/cpp/ql/src/Critical/GlobalUseBeforeInit.ql index 6b286fbcca7f..8c2641bdd5e5 100644 --- a/cpp/ql/src/Critical/GlobalUseBeforeInit.ql +++ b/cpp/ql/src/Critical/GlobalUseBeforeInit.ql @@ -31,7 +31,9 @@ predicate useFunc(GlobalVariable v, Function f) { } predicate uninitialisedBefore(GlobalVariable v, Function f) { - f.hasGlobalName("main") + f.hasGlobalName("main") and + not initialisedAtDeclaration(v) and + not isStdlibVariable(v) or exists(Call call, Function g | uninitialisedBefore(v, g) and @@ -107,9 +109,7 @@ predicate isStdlibVariable(GlobalVariable v) { v.hasGlobalName(["stdin", "stdout from GlobalVariable v, Function f where uninitialisedBefore(v, f) and - useFunc(v, f) and - not initialisedAtDeclaration(v) and - not isStdlibVariable(v) + useFunc(v, f) select f, "The variable '" + v.getName() + " is used in this function but may not be initialized when it is called." From 6cb5db2387a31447a7c9b98db9bee5acfc137aaf Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 16 Apr 2024 13:55:55 +0100 Subject: [PATCH 8/9] C++: Improve change note comments. --- cpp/ql/src/change-notes/2024-04-09-reduce-FP.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/change-notes/2024-04-09-reduce-FP.md b/cpp/ql/src/change-notes/2024-04-09-reduce-FP.md index 0b9b3edbf43b..d2d104d59cd8 100644 --- a/cpp/ql/src/change-notes/2024-04-09-reduce-FP.md +++ b/cpp/ql/src/change-notes/2024-04-09-reduce-FP.md @@ -1,6 +1,5 @@ --- -category: majorAnalysis +category: minorAnalysis --- -* Reduce false positives of `GlobalUseBeforeInit.ql` and `InconsistentNullnessTesting.ql`. -Many global variables that are initialized at declaration will be reported by `GlobalUseBeforeInit.ql`. -When `checked` is in a macro expansion for handling exceptions, it is very likely for `InconsistentNullnessTesting.ql` to report false positives. +* The "Global variable may be used before initialization" query (`cpp/global-use-before-init`) no longer raises an alert on global variables that are initialized when they are declared. +* The "Inconsistent null check of pointer" query (`cpp/inconsistent-nullness-testing`) query no longer raises an alert when the guarded check is in a macro expansion. \ No newline at end of file From e1884c193b0a5be4b2fca36cb0e319ef85be6354 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 16 Apr 2024 14:20:19 +0100 Subject: [PATCH 9/9] C++: Add tests (and fix a missing quote in the alert message). --- cpp/ql/src/Critical/GlobalUseBeforeInit.ql | 2 +- .../GlobalUseBeforeInit.expected | 1 + .../GlobalUseBeforeInit.qlref | 1 + .../Critical/GlobalUseBeforeInit/test.cpp | 38 +++++++++++++++++++ 4 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 cpp/ql/test/query-tests/Critical/GlobalUseBeforeInit/GlobalUseBeforeInit.expected create mode 100644 cpp/ql/test/query-tests/Critical/GlobalUseBeforeInit/GlobalUseBeforeInit.qlref create mode 100644 cpp/ql/test/query-tests/Critical/GlobalUseBeforeInit/test.cpp diff --git a/cpp/ql/src/Critical/GlobalUseBeforeInit.ql b/cpp/ql/src/Critical/GlobalUseBeforeInit.ql index 8c2641bdd5e5..7b27a8529ce9 100644 --- a/cpp/ql/src/Critical/GlobalUseBeforeInit.ql +++ b/cpp/ql/src/Critical/GlobalUseBeforeInit.ql @@ -111,5 +111,5 @@ where uninitialisedBefore(v, f) and useFunc(v, f) select f, - "The variable '" + v.getName() + + "The variable '" + v.getName() + "'" + " is used in this function but may not be initialized when it is called." diff --git a/cpp/ql/test/query-tests/Critical/GlobalUseBeforeInit/GlobalUseBeforeInit.expected b/cpp/ql/test/query-tests/Critical/GlobalUseBeforeInit/GlobalUseBeforeInit.expected new file mode 100644 index 000000000000..789c03f902b3 --- /dev/null +++ b/cpp/ql/test/query-tests/Critical/GlobalUseBeforeInit/GlobalUseBeforeInit.expected @@ -0,0 +1 @@ +| test.cpp:27:5:27:6 | f1 | The variable 'b' is used in this function but may not be initialized when it is called. | diff --git a/cpp/ql/test/query-tests/Critical/GlobalUseBeforeInit/GlobalUseBeforeInit.qlref b/cpp/ql/test/query-tests/Critical/GlobalUseBeforeInit/GlobalUseBeforeInit.qlref new file mode 100644 index 000000000000..a186cc827ec5 --- /dev/null +++ b/cpp/ql/test/query-tests/Critical/GlobalUseBeforeInit/GlobalUseBeforeInit.qlref @@ -0,0 +1 @@ +Critical/GlobalUseBeforeInit.ql diff --git a/cpp/ql/test/query-tests/Critical/GlobalUseBeforeInit/test.cpp b/cpp/ql/test/query-tests/Critical/GlobalUseBeforeInit/test.cpp new file mode 100644 index 000000000000..fcecf6c5c44a --- /dev/null +++ b/cpp/ql/test/query-tests/Critical/GlobalUseBeforeInit/test.cpp @@ -0,0 +1,38 @@ +typedef __builtin_va_list va_list; +typedef struct {} FILE; + +extern FILE * stdin; +extern FILE * stdout; +extern FILE * stderr; + +#define va_start(args, fmt) __builtin_va_start(args,fmt) +#define va_end(args) __builtin_va_end(args); + +int vfprintf (FILE *, const char *, va_list); + +int a = 1; +int b; + +int my_printf(const char * fmt, ...) +{ + va_list vl; + int ret; + va_start(vl, fmt); + ret = vfprintf(stdout, fmt, vl); + ret = vfprintf(stderr, fmt, vl); + va_end(vl); + return ret; +} + +int f1() +{ + my_printf("%d\n", a + 2); + my_printf("%d\n", b + 2); // BAD + return 0; +} + +int main() +{ + int b = f1(); + return 0; +} \ No newline at end of file