From fe85e007b3e4159474fc5f32002866e5de8066b2 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Tue, 15 Oct 2024 13:57:57 +0100 Subject: [PATCH 01/13] C++: Add test for cpp/wrong-type-format-argument --- .../Buildless/WrongTypeFormatArguments.expected | 4 ++++ .../Buildless/WrongTypeFormatArguments.qlref | 1 + .../Format/WrongTypeFormatArguments/Buildless/tests.c | 11 +++++++++++ 3 files changed, 16 insertions(+) create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.expected create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.qlref create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/tests.c diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.expected b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.expected new file mode 100644 index 000000000000..1890aa6b9b41 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.expected @@ -0,0 +1,4 @@ +| tests.c:7:18:7:18 | 1 | This format specifier for type 'char *' does not match the argument type 'int'. | +| tests.c:8:18:8:34 | call to implicit_function | This format specifier for type 'char *' does not match the argument type 'int'. | +| tests.c:9:13:9:13 | 0 | This format specifier for type 'char *' does not match the argument type 'int'. | +| tests.c:10:13:10:13 | 0 | This format specifier for type 'char *' does not match the argument type 'int'. | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.qlref b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.qlref new file mode 100644 index 000000000000..6f557ace55a5 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.qlref @@ -0,0 +1 @@ +Likely Bugs/Format/WrongTypeFormatArguments.ql diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/tests.c b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/tests.c new file mode 100644 index 000000000000..6c0cf81644b3 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/tests.c @@ -0,0 +1,11 @@ +// semmle-extractor-options: --expect_errors + +int printf(const char * format, ...); +int fprintf(); + +int f() { + printf("%s", 1); // BAD - TP + printf("%s", implicit_function()); // BAD (FP) - we should not infer the return type + sprintf(0, "%s", ""); // BAD (FP) + fprintf(0, "%s", ""); // BAD (FP) +} From 853128c9c3126ea34a3fc8c5a09728a3fb1f81cd Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Mon, 14 Oct 2024 17:10:55 +0100 Subject: [PATCH 02/13] C++: Clean up false-positives C++: Change note --- .../semmle/code/cpp/models/implementations/Printf.qll | 4 +++- .../code/cpp/models/interfaces/FormattingFunction.qll | 2 +- .../Likely Bugs/Format/WrongTypeFormatArguments.ql | 3 ++- .../2024-10-15-wrong-type-format-argument.md | 4 ++++ .../Buildless/WrongTypeFormatArguments.expected | 5 +---- .../Format/WrongTypeFormatArguments/Buildless/tests.c | 11 +++++------ 6 files changed, 16 insertions(+), 13 deletions(-) create mode 100644 cpp/ql/src/change-notes/2024-10-15-wrong-type-format-argument.md diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll index 7286552e3ee0..b7e2e4df1c60 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll @@ -50,6 +50,8 @@ private class Fprintf extends FormattingFunction, NonThrowingFunction { override int getFormatParameterIndex() { result = 1 } override int getOutputParameterIndex(boolean isStream) { result = 0 and isStream = true } + + override int getFirstFormatArgumentIndex() { result = 2 } } /** @@ -91,7 +93,7 @@ private class Sprintf extends FormattingFunction, NonThrowingFunction { override int getFirstFormatArgumentIndex() { if this.hasName("__builtin___sprintf_chk") then result = 4 - else result = this.getNumberOfParameters() + else result = this.getNumberOfExplicitParameters() } } diff --git a/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll b/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll index 8ae48181ae18..28e4bfa59652 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll @@ -143,7 +143,7 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction { * from implicit function declarations. If there is some inconsistency in the number * of parameters, then don't return anything. */ - private int getNumberOfExplicitParameters() { + int getNumberOfExplicitParameters() { forex(FunctionDeclarationEntry fde | fde = this.getAnExplicitDeclarationEntry() | result = fde.getNumberOfParameters() ) diff --git a/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql b/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql index e38bfbf562be..6ec448c9e371 100644 --- a/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql +++ b/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql @@ -170,7 +170,8 @@ where ) and not arg.isAffectedByMacro() and not arg.isFromUninstantiatedTemplate(_) and - not actual.getUnspecifiedType() instanceof ErroneousType + not actual.getUnspecifiedType() instanceof ErroneousType and + not arg.(Call).getTarget().getADeclarationEntry().isImplicit() select arg, "This format specifier for type '" + expected.getName() + "' does not match the argument type '" + actual.getUnspecifiedType().getName() + "'." diff --git a/cpp/ql/src/change-notes/2024-10-15-wrong-type-format-argument.md b/cpp/ql/src/change-notes/2024-10-15-wrong-type-format-argument.md new file mode 100644 index 000000000000..56daf8fdbf9b --- /dev/null +++ b/cpp/ql/src/change-notes/2024-10-15-wrong-type-format-argument.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Fixed false positives in the `cpp/wrong-type-format-argument` ("Wrong type of arguments to formatting function") query if there are extraction errors in the function. diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.expected b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.expected index 1890aa6b9b41..15fdd9951ea9 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.expected @@ -1,4 +1 @@ -| tests.c:7:18:7:18 | 1 | This format specifier for type 'char *' does not match the argument type 'int'. | -| tests.c:8:18:8:34 | call to implicit_function | This format specifier for type 'char *' does not match the argument type 'int'. | -| tests.c:9:13:9:13 | 0 | This format specifier for type 'char *' does not match the argument type 'int'. | -| tests.c:10:13:10:13 | 0 | This format specifier for type 'char *' does not match the argument type 'int'. | +| tests.c:6:18:6:18 | 1 | This format specifier for type 'char *' does not match the argument type 'int'. | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/tests.c b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/tests.c index 6c0cf81644b3..b47b607e37f9 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/tests.c +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/tests.c @@ -1,11 +1,10 @@ // semmle-extractor-options: --expect_errors int printf(const char * format, ...); -int fprintf(); -int f() { - printf("%s", 1); // BAD - TP - printf("%s", implicit_function()); // BAD (FP) - we should not infer the return type - sprintf(0, "%s", ""); // BAD (FP) - fprintf(0, "%s", ""); // BAD (FP) +void f() { + printf("%s", 1); // BAD + printf("%s", implicit_function()); // GOOD - we should ignore the type + sprintf(0, "%s", ""); // GOOD + fprintf(0, "%s", ""); // GOOD } From 6a48ad0ee784b496fe128dc0d35eae7a53ac72b8 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Wed, 16 Oct 2024 10:23:39 +0100 Subject: [PATCH 03/13] C++: Implement Expr::mayBeFromImplicitlyDeclaredFunction --- cpp/ql/lib/semmle/code/cpp/exprs/Call.qll | 5 +++++ cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll | 3 +++ cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql | 2 +- .../exprs/implicitly_declared/error_type.expected | 1 + .../library-tests/exprs/implicitly_declared/error_type.ql | 5 +++++ .../library-tests/exprs/implicitly_declared/errors.expected | 2 ++ .../test/library-tests/exprs/implicitly_declared/errors.ql | 4 ++++ .../test/library-tests/exprs/implicitly_declared/implicit.c | 4 ++++ .../library-tests/exprs/implicitly_declared/implicit.cpp | 6 ++++++ .../exprs/implicitly_declared/implicit.expected | 1 + .../library-tests/exprs/implicitly_declared/implicit.ql | 5 +++++ 11 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 cpp/ql/test/library-tests/exprs/implicitly_declared/error_type.expected create mode 100644 cpp/ql/test/library-tests/exprs/implicitly_declared/error_type.ql create mode 100644 cpp/ql/test/library-tests/exprs/implicitly_declared/errors.expected create mode 100644 cpp/ql/test/library-tests/exprs/implicitly_declared/errors.ql create mode 100644 cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.c create mode 100644 cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.cpp create mode 100644 cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.expected create mode 100644 cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.ql diff --git a/cpp/ql/lib/semmle/code/cpp/exprs/Call.qll b/cpp/ql/lib/semmle/code/cpp/exprs/Call.qll index 332cda770bbb..3f192d7194cd 100644 --- a/cpp/ql/lib/semmle/code/cpp/exprs/Call.qll +++ b/cpp/ql/lib/semmle/code/cpp/exprs/Call.qll @@ -300,6 +300,11 @@ class FunctionCall extends Call, @funbindexpr { this.isVirtual() or this.getTarget().getAnAttribute().getName() = "weak" } + + override predicate mayBeFromImplicitlyDeclaredFunction() { + this.getType() instanceof IntType and + this.getTarget().getADeclarationEntry().isImplicit() + } } /** A _user-defined_ unary `operator*` function. */ diff --git a/cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll b/cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll index 91b57049a54e..0c42e5cbb89e 100644 --- a/cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll +++ b/cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll @@ -534,6 +534,9 @@ class Expr extends StmtParent, @expr { /** Gets the function containing this control-flow node. */ override Function getControlFlowScope() { result = this.getEnclosingFunction() } + + /** Holds if this expression could be the return value of an implicitly declared function. */ + predicate mayBeFromImplicitlyDeclaredFunction() { none() } } /** diff --git a/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql b/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql index 6ec448c9e371..ec77339a6081 100644 --- a/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql +++ b/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql @@ -171,7 +171,7 @@ where not arg.isAffectedByMacro() and not arg.isFromUninstantiatedTemplate(_) and not actual.getUnspecifiedType() instanceof ErroneousType and - not arg.(Call).getTarget().getADeclarationEntry().isImplicit() + not arg.mayBeFromImplicitlyDeclaredFunction() select arg, "This format specifier for type '" + expected.getName() + "' does not match the argument type '" + actual.getUnspecifiedType().getName() + "'." diff --git a/cpp/ql/test/library-tests/exprs/implicitly_declared/error_type.expected b/cpp/ql/test/library-tests/exprs/implicitly_declared/error_type.expected new file mode 100644 index 000000000000..b7974b1b2623 --- /dev/null +++ b/cpp/ql/test/library-tests/exprs/implicitly_declared/error_type.expected @@ -0,0 +1 @@ +| file://:0:0:0:0 | | diff --git a/cpp/ql/test/library-tests/exprs/implicitly_declared/error_type.ql b/cpp/ql/test/library-tests/exprs/implicitly_declared/error_type.ql new file mode 100644 index 000000000000..81e1f1fefa7c --- /dev/null +++ b/cpp/ql/test/library-tests/exprs/implicitly_declared/error_type.ql @@ -0,0 +1,5 @@ +import cpp + +from Expr e +where e.getType() instanceof ErroneousType +select e diff --git a/cpp/ql/test/library-tests/exprs/implicitly_declared/errors.expected b/cpp/ql/test/library-tests/exprs/implicitly_declared/errors.expected new file mode 100644 index 000000000000..84a1b0c7df25 --- /dev/null +++ b/cpp/ql/test/library-tests/exprs/implicitly_declared/errors.expected @@ -0,0 +1,2 @@ +| file://:0:0:0:0 | There was an error during this compilation | +| implicit.cpp:5:5:5:5 | identifier 'g' is undefined | diff --git a/cpp/ql/test/library-tests/exprs/implicitly_declared/errors.ql b/cpp/ql/test/library-tests/exprs/implicitly_declared/errors.ql new file mode 100644 index 000000000000..3fa864748e16 --- /dev/null +++ b/cpp/ql/test/library-tests/exprs/implicitly_declared/errors.ql @@ -0,0 +1,4 @@ +import cpp + +from Diagnostic d +select d diff --git a/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.c b/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.c new file mode 100644 index 000000000000..5cfb42c79ba0 --- /dev/null +++ b/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.c @@ -0,0 +1,4 @@ +void f() { + f(); + g(); +} diff --git a/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.cpp b/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.cpp new file mode 100644 index 000000000000..f64bf61753fa --- /dev/null +++ b/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.cpp @@ -0,0 +1,6 @@ +// semmle-extractor-options: --expect_errors + +void f() { + f(); + g(); +} diff --git a/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.expected b/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.expected new file mode 100644 index 000000000000..8f626de5e7cc --- /dev/null +++ b/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.expected @@ -0,0 +1 @@ +| implicit.c:3:5:3:5 | call to g | diff --git a/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.ql b/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.ql new file mode 100644 index 000000000000..99683f847702 --- /dev/null +++ b/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.ql @@ -0,0 +1,5 @@ +import cpp + +from Expr e +where e.mayBeFromImplicitlyDeclaredFunction() +select e From d88a674a1565ac5fecc6131ee30519c6f65c55a1 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Wed, 16 Oct 2024 10:27:54 +0100 Subject: [PATCH 04/13] C++: Change note for mayBeFromImplicitlyDeclaredFunction --- cpp/ql/lib/change-notes/2014-10-16-implicitly-declared-fns.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 cpp/ql/lib/change-notes/2014-10-16-implicitly-declared-fns.md diff --git a/cpp/ql/lib/change-notes/2014-10-16-implicitly-declared-fns.md b/cpp/ql/lib/change-notes/2014-10-16-implicitly-declared-fns.md new file mode 100644 index 000000000000..3fc424fe34ee --- /dev/null +++ b/cpp/ql/lib/change-notes/2014-10-16-implicitly-declared-fns.md @@ -0,0 +1,4 @@ +--- +category: feature +--- +* Added the predicate `mayBeFromImplicitlyDeclaredFunction()` to the `Expr` class to represent expressions that may be the return value of an implicitly declared C function. From ceceee1947032249885ab58c4649e93c18e1190a Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Wed, 16 Oct 2024 13:46:06 +0100 Subject: [PATCH 05/13] C++: Add test for mixed implicit/explicit function declarations --- .../exprs/implicitly_declared/functions.expected | 6 ++++++ .../library-tests/exprs/implicitly_declared/functions.ql | 5 +++++ .../library-tests/exprs/implicitly_declared/implicit2.c | 1 + 3 files changed, 12 insertions(+) create mode 100644 cpp/ql/test/library-tests/exprs/implicitly_declared/functions.expected create mode 100644 cpp/ql/test/library-tests/exprs/implicitly_declared/functions.ql create mode 100644 cpp/ql/test/library-tests/exprs/implicitly_declared/implicit2.c diff --git a/cpp/ql/test/library-tests/exprs/implicitly_declared/functions.expected b/cpp/ql/test/library-tests/exprs/implicitly_declared/functions.expected new file mode 100644 index 000000000000..e7142fb7df2d --- /dev/null +++ b/cpp/ql/test/library-tests/exprs/implicitly_declared/functions.expected @@ -0,0 +1,6 @@ +| implicit2.c:1:7:1:7 | g | file://:0:0:0:0 | float | +| implicit2.c:1:7:1:7 | g | file://:0:0:0:0 | int | +| implicit.c:1:6:1:6 | f | file://:0:0:0:0 | void | +| implicit.c:3:5:3:5 | g | file://:0:0:0:0 | float | +| implicit.c:3:5:3:5 | g | file://:0:0:0:0 | int | +| implicit.cpp:3:6:3:6 | f | file://:0:0:0:0 | void | diff --git a/cpp/ql/test/library-tests/exprs/implicitly_declared/functions.ql b/cpp/ql/test/library-tests/exprs/implicitly_declared/functions.ql new file mode 100644 index 000000000000..71d120935a5d --- /dev/null +++ b/cpp/ql/test/library-tests/exprs/implicitly_declared/functions.ql @@ -0,0 +1,5 @@ +import cpp + +from Function fn +where fn.fromSource() +select fn, fn.getType() diff --git a/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit2.c b/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit2.c new file mode 100644 index 000000000000..0f4c2a489e39 --- /dev/null +++ b/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit2.c @@ -0,0 +1 @@ +float g(); From 9758e023f90f668dbd27711419d5ecc480e40f06 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Wed, 16 Oct 2024 13:47:06 +0100 Subject: [PATCH 06/13] C++: Remove redundant test --- cpp/ql/lib/semmle/code/cpp/exprs/Call.qll | 1 - 1 file changed, 1 deletion(-) diff --git a/cpp/ql/lib/semmle/code/cpp/exprs/Call.qll b/cpp/ql/lib/semmle/code/cpp/exprs/Call.qll index 3f192d7194cd..167ddda330e3 100644 --- a/cpp/ql/lib/semmle/code/cpp/exprs/Call.qll +++ b/cpp/ql/lib/semmle/code/cpp/exprs/Call.qll @@ -302,7 +302,6 @@ class FunctionCall extends Call, @funbindexpr { } override predicate mayBeFromImplicitlyDeclaredFunction() { - this.getType() instanceof IntType and this.getTarget().getADeclarationEntry().isImplicit() } } From 5315a5cfbf34641addabffa777e662bd147fce6a Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Wed, 16 Oct 2024 15:47:34 +0100 Subject: [PATCH 07/13] C++: Tweak test --- .../Buildless/WrongTypeFormatArguments.expected | 2 +- .../Format/WrongTypeFormatArguments/Buildless/tests.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.expected b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.expected index 15fdd9951ea9..745f2f790f79 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.expected @@ -1 +1 @@ -| tests.c:6:18:6:18 | 1 | This format specifier for type 'char *' does not match the argument type 'int'. | +| tests.c:7:18:7:18 | 1 | This format specifier for type 'char *' does not match the argument type 'int'. | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/tests.c b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/tests.c index b47b607e37f9..81698c497c57 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/tests.c +++ b/cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/tests.c @@ -1,6 +1,7 @@ // semmle-extractor-options: --expect_errors int printf(const char * format, ...); +int fprintf(); void f() { printf("%s", 1); // BAD From 4341fab79472324ef6e39e09329cb563a3e194c8 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Thu, 17 Oct 2024 10:50:44 +0100 Subject: [PATCH 08/13] C++: Reject invalid results from getFirstFormatArgumentIndex() --- cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll | 2 -- .../semmle/code/cpp/models/interfaces/FormattingFunction.qll | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll index b7e2e4df1c60..cab918662070 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll @@ -50,8 +50,6 @@ private class Fprintf extends FormattingFunction, NonThrowingFunction { override int getFormatParameterIndex() { result = 1 } override int getOutputParameterIndex(boolean isStream) { result = 0 and isStream = true } - - override int getFirstFormatArgumentIndex() { result = 2 } } /** diff --git a/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll b/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll index 28e4bfa59652..53b08afacfd5 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll @@ -125,6 +125,7 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction { // The formatting function either has a definition in the snapshot, or all // `DeclarationEntry`s agree on the number of parameters (otherwise we don't // really know the correct number) + result > 0 and // Avoid invalid declarations if this.hasDefinition() then result = this.getDefinition().getNumberOfParameters() else result = this.getNumberOfExplicitParameters() From 419780591a5d6ff37d3799d0b749b43eceb03bf2 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Fri, 18 Oct 2024 14:52:54 +0100 Subject: [PATCH 09/13] C++: Resolve firstFormatArgumentIndex in FormattingFunction CP --- cpp/ql/lib/semmle/code/cpp/Function.qll | 8 ++++ .../cpp/models/implementations/Printf.qll | 2 +- .../models/interfaces/FormattingFunction.qll | 41 ++++++------------- 3 files changed, 22 insertions(+), 29 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/Function.qll b/cpp/ql/lib/semmle/code/cpp/Function.qll index 7cfa779338f7..18b7c21dbe23 100644 --- a/cpp/ql/lib/semmle/code/cpp/Function.qll +++ b/cpp/ql/lib/semmle/code/cpp/Function.qll @@ -230,6 +230,14 @@ class Function extends Declaration, ControlFlowNode, AccessHolder, @function { ) } + /** + * Gets a non-implicit function declaration entry. + */ + FunctionDeclarationEntry getAnExplicitDeclarationEntry() { + result = this.getADeclarationEntry() and + not result.isImplicit() + } + private predicate declEntry(FunctionDeclarationEntry fde) { fun_decls(unresolveElement(fde), underlyingElement(this), _, _, _) and // If one .cpp file specializes a function, and another calls the diff --git a/cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll b/cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll index cab918662070..9c3bfb4f35ec 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll @@ -91,7 +91,7 @@ private class Sprintf extends FormattingFunction, NonThrowingFunction { override int getFirstFormatArgumentIndex() { if this.hasName("__builtin___sprintf_chk") then result = 4 - else result = this.getNumberOfExplicitParameters() + else result = super.getFirstFormatArgumentIndex() } } diff --git a/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll b/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll index 53b08afacfd5..96d2f37d0efa 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll @@ -42,6 +42,18 @@ private Type getAFormatterWideTypeOrDefault() { * A standard library function that uses a `printf`-like formatting string. */ abstract class FormattingFunction extends ArrayFunction, TaintFunction { + int firstFormatArgumentIndex; + + FormattingFunction() { + firstFormatArgumentIndex > 0 and + if this.hasDefinition() + then firstFormatArgumentIndex = this.getDefinition().getNumberOfParameters() + else + forex(FunctionDeclarationEntry fde | fde = this.getAnExplicitDeclarationEntry() | + firstFormatArgumentIndex = fde.getNumberOfParameters() + ) + } + /** Gets the position at which the format parameter occurs. */ abstract int getFormatParameterIndex(); @@ -121,34 +133,7 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction { * the first format specifier in the format string. We ignore all * implicit function definitions. */ - int getFirstFormatArgumentIndex() { - // The formatting function either has a definition in the snapshot, or all - // `DeclarationEntry`s agree on the number of parameters (otherwise we don't - // really know the correct number) - result > 0 and // Avoid invalid declarations - if this.hasDefinition() - then result = this.getDefinition().getNumberOfParameters() - else result = this.getNumberOfExplicitParameters() - } - - /** - * Gets a non-implicit function declaration entry. - */ - private FunctionDeclarationEntry getAnExplicitDeclarationEntry() { - result = this.getADeclarationEntry() and - not result.isImplicit() - } - - /** - * Gets the number of parameters, excluding any parameters that have been defined - * from implicit function declarations. If there is some inconsistency in the number - * of parameters, then don't return anything. - */ - int getNumberOfExplicitParameters() { - forex(FunctionDeclarationEntry fde | fde = this.getAnExplicitDeclarationEntry() | - result = fde.getNumberOfParameters() - ) - } + int getFirstFormatArgumentIndex() { result = firstFormatArgumentIndex } /** * Gets the position of the buffer size argument, if any. From 0fcabc4e61f922f9f4b8e42427f9f240c30ce3ae Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Fri, 18 Oct 2024 15:56:08 +0100 Subject: [PATCH 10/13] C++: Move mayBeFromImplicitlyDeclaredFunction to Call --- cpp/ql/lib/semmle/code/cpp/exprs/Call.qll | 9 +++++---- cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll | 3 --- .../src/Likely Bugs/Format/WrongTypeFormatArguments.ql | 2 +- .../library-tests/exprs/implicitly_declared/implicit.ql | 6 +++--- 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/exprs/Call.qll b/cpp/ql/lib/semmle/code/cpp/exprs/Call.qll index 167ddda330e3..03c3e8a33714 100644 --- a/cpp/ql/lib/semmle/code/cpp/exprs/Call.qll +++ b/cpp/ql/lib/semmle/code/cpp/exprs/Call.qll @@ -149,6 +149,11 @@ class Call extends Expr, NameQualifiableElement, TCall { variableAddressEscapesTreeNonConst(va, this.getQualifier().getFullyConverted()) and i = -1 } + + /** Holds if this expression could be the return value of an implicitly declared function. */ + predicate mayBeFromImplicitlyDeclaredFunction() { + this.getTarget().getADeclarationEntry().isImplicit() + } } /** @@ -300,10 +305,6 @@ class FunctionCall extends Call, @funbindexpr { this.isVirtual() or this.getTarget().getAnAttribute().getName() = "weak" } - - override predicate mayBeFromImplicitlyDeclaredFunction() { - this.getTarget().getADeclarationEntry().isImplicit() - } } /** A _user-defined_ unary `operator*` function. */ diff --git a/cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll b/cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll index 0c42e5cbb89e..91b57049a54e 100644 --- a/cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll +++ b/cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll @@ -534,9 +534,6 @@ class Expr extends StmtParent, @expr { /** Gets the function containing this control-flow node. */ override Function getControlFlowScope() { result = this.getEnclosingFunction() } - - /** Holds if this expression could be the return value of an implicitly declared function. */ - predicate mayBeFromImplicitlyDeclaredFunction() { none() } } /** diff --git a/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql b/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql index ec77339a6081..027f4caa8ae4 100644 --- a/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql +++ b/cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql @@ -171,7 +171,7 @@ where not arg.isAffectedByMacro() and not arg.isFromUninstantiatedTemplate(_) and not actual.getUnspecifiedType() instanceof ErroneousType and - not arg.mayBeFromImplicitlyDeclaredFunction() + not arg.(Call).mayBeFromImplicitlyDeclaredFunction() select arg, "This format specifier for type '" + expected.getName() + "' does not match the argument type '" + actual.getUnspecifiedType().getName() + "'." diff --git a/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.ql b/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.ql index 99683f847702..fe640c83de9d 100644 --- a/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.ql +++ b/cpp/ql/test/library-tests/exprs/implicitly_declared/implicit.ql @@ -1,5 +1,5 @@ import cpp -from Expr e -where e.mayBeFromImplicitlyDeclaredFunction() -select e +from Call c +where c.mayBeFromImplicitlyDeclaredFunction() +select c From c5a082fd8e51c69fdfacec137a9a3bdbf70782d2 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Fri, 18 Oct 2024 19:45:29 +0100 Subject: [PATCH 11/13] C++: Fix CWE-022 --- .../test/query-tests/Security/CWE/CWE-022/semmle/tests/stdlib.h | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/stdlib.h b/cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/stdlib.h index 53344da57d6d..8c0cb21482be 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/stdlib.h +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/stdlib.h @@ -17,3 +17,4 @@ void *malloc(size_t size); double strtod(const char *ptr, char **endptr); char *getenv(const char *name); ssize_t read(int fd, void *buffer, size_t count); +int snprintf(char *s, size_t n, const char *format, ...); From f37be68067db3af9a0f6f9a26569a378f776db42 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Wed, 23 Oct 2024 14:35:32 +0100 Subject: [PATCH 12/13] C++: Handle builtin FormattingFunctions better --- .../code/cpp/models/interfaces/FormattingFunction.qll | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll b/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll index 96d2f37d0efa..ce65a65319a8 100644 --- a/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll +++ b/cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll @@ -49,9 +49,12 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction { if this.hasDefinition() then firstFormatArgumentIndex = this.getDefinition().getNumberOfParameters() else - forex(FunctionDeclarationEntry fde | fde = this.getAnExplicitDeclarationEntry() | - firstFormatArgumentIndex = fde.getNumberOfParameters() - ) + if this instanceof BuiltInFunction + then firstFormatArgumentIndex = this.getNumberOfParameters() + else + forex(FunctionDeclarationEntry fde | fde = this.getAnExplicitDeclarationEntry() | + firstFormatArgumentIndex = fde.getNumberOfParameters() + ) } /** Gets the position at which the format parameter occurs. */ From 421413a6541dec88206d3505836fef0def023aa0 Mon Sep 17 00:00:00 2001 From: Calum Grant Date: Wed, 23 Oct 2024 14:46:00 +0100 Subject: [PATCH 13/13] C++: Update change notes --- cpp/ql/lib/change-notes/2014-10-16-implicitly-declared-fns.md | 3 ++- .../src/change-notes/2024-10-15-wrong-type-format-argument.md | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cpp/ql/lib/change-notes/2014-10-16-implicitly-declared-fns.md b/cpp/ql/lib/change-notes/2014-10-16-implicitly-declared-fns.md index 3fc424fe34ee..3a40cef48485 100644 --- a/cpp/ql/lib/change-notes/2014-10-16-implicitly-declared-fns.md +++ b/cpp/ql/lib/change-notes/2014-10-16-implicitly-declared-fns.md @@ -1,4 +1,5 @@ --- category: feature --- -* Added the predicate `mayBeFromImplicitlyDeclaredFunction()` to the `Expr` class to represent expressions that may be the return value of an implicitly declared C function. +* Added the predicate `mayBeFromImplicitlyDeclaredFunction()` to the `Call` class to represent calls that may be the return value of an implicitly declared C function. +* Added the predicate `getAnExplicitDeclarationEntry()` to the `Function` class to get a `FunctionDeclarationEntry` that is not implicit. diff --git a/cpp/ql/src/change-notes/2024-10-15-wrong-type-format-argument.md b/cpp/ql/src/change-notes/2024-10-15-wrong-type-format-argument.md index 56daf8fdbf9b..a7a85535d346 100644 --- a/cpp/ql/src/change-notes/2024-10-15-wrong-type-format-argument.md +++ b/cpp/ql/src/change-notes/2024-10-15-wrong-type-format-argument.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* Fixed false positives in the `cpp/wrong-type-format-argument` ("Wrong type of arguments to formatting function") query if there are extraction errors in the function. +* Remove results from the `cpp/wrong-type-format-argument` ("Wrong type of arguments to formatting function") query if the argument is the return value of an implicitly declared function.