From 64ed9305d39a470406e1bcb699bd6ddbd4687434 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Thu, 17 Jan 2019 10:45:10 -0800 Subject: [PATCH 1/5] C++: new query for futile arguments to C functions --- .../Likely Bugs/Likely Typos/FutileParams.c | 11 +++++++++ .../Likely Typos/FutileParams.qhelp | 24 +++++++++++++++++++ .../Likely Bugs/Likely Typos/FutileParams.ql | 18 ++++++++++++++ .../FutileParams/FutileParams.expected | 3 +++ .../FutileParams/FutileParams.qlref | 1 + .../Likely Typos/FutileParams/test.c | 18 ++++++++++++++ .../Likely Typos/FutileParams/test.cpp | 8 +++++++ 7 files changed, 83 insertions(+) create mode 100644 cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.c create mode 100644 cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp create mode 100644 cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/FutileParams.expected create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/FutileParams.qlref create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.c create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.cpp diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.c b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.c new file mode 100644 index 000000000000..1a827a6c90a3 --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.c @@ -0,0 +1,11 @@ +void no_argument(); + +void one_argument(int x); + +void calls() { + no_argument(1) // BAD: `no_argument` will accept and ignore the argument + + one_argument(1); // GOOD: `one_argument` will accept and use the argument + + no_argument(); // GOOD: `no_argument` has not been passed an argument +} \ No newline at end of file diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp new file mode 100644 index 000000000000..b5fdeeb7f31b --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp @@ -0,0 +1,24 @@ + + + + + +

A function is called with arguments despite having an empty parameter list. This may indicate +that the incorrect function is being called, or that the author misunderstood the function.

+ + + +

Call the function without arguments, or call a different function that expects the arguments +being passed.

+ + + + + + + +
  • SEI CERT C++ Coding Standard: DCL20-C. Explicitly specify void when a function accepts no arguments
    • +     +     diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql new file mode 100644 index 000000000000..ec7260d5d7fb --- /dev/null +++ b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql @@ -0,0 +1,18 @@ +/** + * @name Non-empty call to function declared without parameters + * @description A call to a function declared without parameters has arguments, which may indicate + * that the code does not follow the author's intent. + * @kind problem + * @problem.severity warning + */ + +import cpp + +from Function f, FunctionCall fc +where fc.getTarget() = f + and f.getNumberOfParameters() = 0 + and not f.isVarargs() + and fc.getNumberOfArguments() != 0 + and not f instanceof BuiltInFunction + and exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() | not fde.isImplicit()) +select fc, "This call has arguments, but $@ is not declared with any parameters.", f, f.toString() diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/FutileParams.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/FutileParams.expected new file mode 100644 index 000000000000..7ba6f06b2e4d --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/FutileParams.expected @@ -0,0 +1,3 @@ +| test.c:7:3:7:5 | call to foo | This call has arguments, but $@ is not declared with any parameters. | test.c:1:6:1:8 | foo | foo | +| test.c:13:3:13:19 | call to not_yet_declared1 | This call has arguments, but $@ is not declared with any parameters. | test.c:13:3:13:3 | not_yet_declared1 | not_yet_declared1 | +| test.c:13:3:13:19 | call to not_yet_declared1 | This call has arguments, but $@ is not declared with any parameters. | test.c:17:6:17:22 | not_yet_declared1 | not_yet_declared1 | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/FutileParams.qlref b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/FutileParams.qlref new file mode 100644 index 000000000000..f6dbe907dc59 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/FutileParams.qlref @@ -0,0 +1 @@ +Likely Bugs/Likely Typos/FutileParams.ql diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.c b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.c new file mode 100644 index 000000000000..67bb77266936 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.c @@ -0,0 +1,18 @@ +void foo(); +void bar(void); +void baz(int); + +void test() { + foo(); // GOOD + foo(1); // BAD + bar(); // GOOD + baz(1); // GOOD + + undeclared(1); // GOOD + + not_yet_declared1(1); // BAD + not_yet_declared2(1); // GOOD +} + +void not_yet_declared1(); +void not_yet_declared2(int); \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.cpp new file mode 100644 index 000000000000..855fdaea3fd3 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.cpp @@ -0,0 +1,8 @@ +void cpp_varargs(...); +void bar(); + +void test() { + cpp_varargs(); // GOOD + cpp_varargs(1); // GOOD + __builtin_constant_p("something"); // GOOD: builtin +} \ No newline at end of file From fa02042fdaab92628b39f6da7017e07020233793 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Wed, 23 Jan 2019 11:42:44 -0800 Subject: [PATCH 2/5] C++: add more tests and rename test functions --- .../FutileParams/FutileParams.expected | 6 ++--- .../Likely Typos/FutileParams/test.c | 27 +++++++++++++------ 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/FutileParams.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/FutileParams.expected index 7ba6f06b2e4d..d6dc2222be42 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/FutileParams.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/FutileParams.expected @@ -1,3 +1,3 @@ -| test.c:7:3:7:5 | call to foo | This call has arguments, but $@ is not declared with any parameters. | test.c:1:6:1:8 | foo | foo | -| test.c:13:3:13:19 | call to not_yet_declared1 | This call has arguments, but $@ is not declared with any parameters. | test.c:13:3:13:3 | not_yet_declared1 | not_yet_declared1 | -| test.c:13:3:13:19 | call to not_yet_declared1 | This call has arguments, but $@ is not declared with any parameters. | test.c:17:6:17:22 | not_yet_declared1 | not_yet_declared1 | +| test.c:8:3:8:16 | call to declared_empty | This call has arguments, but $@ is not declared with any parameters. | test.c:1:6:1:19 | declared_empty | declared_empty | +| test.c:14:3:14:19 | call to not_yet_declared1 | This call has arguments, but $@ is not declared with any parameters. | test.c:14:3:14:3 | not_yet_declared1 | not_yet_declared1 | +| test.c:14:3:14:19 | call to not_yet_declared1 | This call has arguments, but $@ is not declared with any parameters. | test.c:25:6:25:22 | not_yet_declared1 | not_yet_declared1 | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.c b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.c index 67bb77266936..0f40e5714165 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.c +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.c @@ -1,18 +1,29 @@ -void foo(); -void bar(void); -void baz(int); +void declared_empty(); +void declared_void(void); +void declared_with(int); +void declared_empty_defined_with(); void test() { - foo(); // GOOD - foo(1); // BAD - bar(); // GOOD - baz(1); // GOOD + declared_empty(); // GOOD + declared_empty(1); // BAD + declared_void(); // GOOD + declared_with(1); // GOOD undeclared(1); // GOOD not_yet_declared1(1); // BAD not_yet_declared2(1); // GOOD + + declared_empty_defined_with(); // BAD + declared_empty_defined_with(1); // GOOD + + int x; + declared_empty_defined_with(&x); // BAD + declared_empty_defined_with(x, x); // BAD } void not_yet_declared1(); -void not_yet_declared2(int); \ No newline at end of file +void not_yet_declared2(int); +void declared_empty_defined_with(int x) { + // do nothing +} From 44d8e6b6e28ab08497dbaffa43985f75e91f9b19 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Thu, 24 Jan 2019 10:50:56 -0800 Subject: [PATCH 3/5] C++: respond to PR comments --- cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp | 5 +++++ .../Likely Bugs/Likely Typos/FutileParams/test.c | 6 +++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp index b5fdeeb7f31b..0316bfdfa36c 100644 --- a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp +++ b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp @@ -8,6 +8,11 @@

    A function is called with arguments despite having an empty parameter list. This may indicate that the incorrect function is being called, or that the author misunderstood the function.

    +

    In C, a function declared with an empty parameter list `()` is considered to have an unknown +parameter list, and therefore can be called with any set of arguments. To declare a function +which takes no arguments, you must use `(void)` as the parameter list in any forward declarations. +In C++, either style of declaration will be considered to take no arguments.

    +

    Call the function without arguments, or call a different function that expects the arguments diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.c b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.c index 0f40e5714165..9589d2a9ffc9 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.c +++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.c @@ -14,12 +14,12 @@ void test() { not_yet_declared1(1); // BAD not_yet_declared2(1); // GOOD - declared_empty_defined_with(); // BAD + declared_empty_defined_with(); // BAD [NOT DETECTED] declared_empty_defined_with(1); // GOOD int x; - declared_empty_defined_with(&x); // BAD - declared_empty_defined_with(x, x); // BAD + declared_empty_defined_with(&x); // BAD [NOT DETECTED] + declared_empty_defined_with(x, x); // BAD [NOT DETECTED] } void not_yet_declared1(); From 54fdf9f29d55b9fb79865654ea3623b9c96aed8a Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Mon, 28 Jan 2019 09:34:45 -0800 Subject: [PATCH 4/5] C++/Docs: respond to doc comments on PR --- cpp/config/suites/c/correctness | 3 ++- cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp | 2 +- cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql | 4 ++++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/cpp/config/suites/c/correctness b/cpp/config/suites/c/correctness index e53ccda11870..442e618854d4 100644 --- a/cpp/config/suites/c/correctness +++ b/cpp/config/suites/c/correctness @@ -7,7 +7,7 @@ + semmlecode-cpp-queries/Likely Bugs/Conversion/NonzeroValueCastToPointer.ql: /Correctness/Dangerous Conversions + semmlecode-cpp-queries/Likely Bugs/Conversion/ImplicitDowncastFromBitfield.ql: /Correctness/Dangerous Conversions + semmlecode-cpp-queries/Security/CWE/CWE-253/HResultBooleanConversion.ql: /Correctness/Dangerous Conversions - # Consistent Use + # Consistent Use + semmlecode-cpp-queries/Critical/ReturnValueIgnored.ql: /Correctness/Consistent Use + semmlecode-cpp-queries/Likely Bugs/InconsistentCheckReturnNull.ql: /Correctness/Consistent Use + semmlecode-cpp-queries/Likely Bugs/InconsistentCallOnResult.ql: /Correctness/Consistent Use @@ -15,6 +15,7 @@ + semmlecode-cpp-queries/Likely Bugs/Likely Typos/AssignWhereCompareMeant.ql: /Correctness/Common Errors + semmlecode-cpp-queries/Likely Bugs/Likely Typos/CompareWhereAssignMeant.ql: /Correctness/Common Errors + semmlecode-cpp-queries/Likely Bugs/Likely Typos/ExprHasNoEffect.ql: /Correctness/Common Errors ++ semmlecode-cpp-queries/Likely Bugs/Likely Typos/FutileParams.ql: /Correctness/Common Errors + semmlecode-cpp-queries/Likely Bugs/Likely Typos/ShortCircuitBitMask.ql: /Correctness/Common Errors + semmlecode-cpp-queries/Likely Bugs/Likely Typos/MissingEnumCaseInSwitch.ql: /Correctness/Common Errors + semmlecode-cpp-queries/Likely Bugs/Arithmetic/FloatComparison.ql: /Correctness/Common Errors diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp index 0316bfdfa36c..6c4fc404fea5 100644 --- a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp +++ b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp @@ -11,7 +11,7 @@ that the incorrect function is being called, or that the author misunderstood th

    In C, a function declared with an empty parameter list `()` is considered to have an unknown parameter list, and therefore can be called with any set of arguments. To declare a function which takes no arguments, you must use `(void)` as the parameter list in any forward declarations. -In C++, either style of declaration will be considered to take no arguments.

    +In C++, either style of declaration indicates that the function accepts no arguments.

    diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql index ec7260d5d7fb..bb5a99a69bc9 100644 --- a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql +++ b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql @@ -4,6 +4,10 @@ * that the code does not follow the author's intent. * @kind problem * @problem.severity warning + * @precision very-high + * @id cpp/futile-params + * @tags correctness + * maintainability */ import cpp From 9642a78bdefb2c1551c6fcbec6e02f1fa593b006 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Mon, 28 Jan 2019 09:40:19 -0800 Subject: [PATCH 5/5] C++: add FutileParams.ql to C++ suite In theory this query will produce no results on C++ code; in practice, I suspect the "cpp" suite is often run on code compiled as C, so it is likely to be worth running anyways. --- cpp/config/suites/cpp/correctness | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/config/suites/cpp/correctness b/cpp/config/suites/cpp/correctness index ceb15f552606..00ad7a1ad07d 100644 --- a/cpp/config/suites/cpp/correctness +++ b/cpp/config/suites/cpp/correctness @@ -8,7 +8,7 @@ + semmlecode-cpp-queries/Likely Bugs/Conversion/ImplicitDowncastFromBitfield.ql: /Correctness/Dangerous Conversions + semmlecode-cpp-queries/Likely Bugs/Conversion/CastArrayPointerArithmetic.ql: /Correctness/Dangerous Conversions + semmlecode-cpp-queries/Security/CWE/CWE-253/HResultBooleanConversion.ql: /Correctness/Dangerous Conversions - # Consistent Use + # Consistent Use + semmlecode-cpp-queries/Critical/ReturnValueIgnored.ql: /Correctness/Consistent Use + semmlecode-cpp-queries/Likely Bugs/InconsistentCheckReturnNull.ql: /Correctness/Consistent Use + semmlecode-cpp-queries/Likely Bugs/InconsistentCallOnResult.ql: /Correctness/Consistent Use @@ -16,6 +16,7 @@ + semmlecode-cpp-queries/Likely Bugs/Likely Typos/AssignWhereCompareMeant.ql: /Correctness/Common Errors + semmlecode-cpp-queries/Likely Bugs/Likely Typos/CompareWhereAssignMeant.ql: /Correctness/Common Errors + semmlecode-cpp-queries/Likely Bugs/Likely Typos/ExprHasNoEffect.ql: /Correctness/Common Errors ++ semmlecode-cpp-queries/Likely Bugs/Likely Typos/FutileParams.ql: /Correctness/Common Errors + semmlecode-cpp-queries/Likely Bugs/Likely Typos/ShortCircuitBitMask.ql: /Correctness/Common Errors + semmlecode-cpp-queries/Likely Bugs/Likely Typos/MissingEnumCaseInSwitch.ql: /Correctness/Common Errors + semmlecode-cpp-queries/Likely Bugs/Arithmetic/FloatComparison.ql: /Correctness/Common Errors