From 43549e639760c1fef87687b43531bbe94eca27b2 Mon Sep 17 00:00:00 2001 From: Ziemowit Laski Date: Mon, 18 Mar 2019 11:03:27 -0700 Subject: [PATCH 1/5] Change names of parameters to memcpy(), as per Geoff. --- .../Memory Management/AllocaInLoop/AllocaInLoop1.cpp | 2 +- .../Memory Management/AllocaInLoop/AllocaInLoop1ms.cpp | 2 +- .../Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop2.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1.cpp b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1.cpp index 7d5ac6d0f0c3..54a93c511bee 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1.cpp @@ -15,7 +15,7 @@ void *__builtin_alloca(int sz); typedef unsigned long size_t; int printf(const char *restrict format, ...); -void *memcpy(void *restrict dst, const void *restrict src, size_t len); +void *memcpy(void *restrict s1, const void *restrict s2, size_t n); // case 1: alloca directly contained in an unbounded loop void foo(const struct vtype* vec, int count) { diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1ms.cpp b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1ms.cpp index 4c698bfdc269..45b36161821d 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1ms.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1ms.cpp @@ -12,7 +12,7 @@ extern int w1, w2; #endif int printf(const char *restrict format, ...); -void *memcpy(void *restrict dst, const void *restrict src, size_t len); +void *memcpy(void *restrict s1, const void *restrict s2, size_t n); // case 1: _alloca directly contained in an unbounded loop void foo(const struct vtype* vec, int count) { diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop2.c b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop2.c index 0f137b66056e..eeeafb2f17ff 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop2.c +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop2.c @@ -2,7 +2,7 @@ int printf(const char *restrict format, ...); int sprintf(char *restrict buf, const char *restrict format, ...); typedef unsigned long size_t; -void *memcpy(void *restrict dst, const void *restrict src, size_t len); +void *memcpy(void *restrict s1, const void *restrict s2, size_t n); void *malloc(size_t sz); void free(void *ptr); From 96249b88380c48dd345681e34884bbe58d7ee238 Mon Sep 17 00:00:00 2001 From: Ziemowit Laski Date: Mon, 18 Mar 2019 16:10:35 -0700 Subject: [PATCH 2/5] [CPP-340] Create QL query for function call argument count mismatches. Update QHELP file, test and test results. --- .../Likely Typos/FutileParams.qhelp | 15 +++++++++++---- .../Likely Bugs/Likely Typos/FutileParams.ql | 19 +++++++++++-------- .../FutileParams/FutileParams.expected | 7 ++++--- .../Likely Typos/FutileParams/test.c | 10 ++++++---- 4 files changed, 32 insertions(+), 19 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp index d0d888a9aac7..0c1e345c04fa 100644 --- a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp +++ b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp @@ -5,18 +5,25 @@ -

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.

+

A function is called with arguments despite having been declared with an empty parameter list. +In addition, we were either unable to find a definition of the function, or found one with +an incompatible number of parameters.

+ +

This may indicate that an incorrect function is being called, or that the signature (parameter list) +of the called function is not known to the author.

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 indicates that the function accepts no arguments.

+In C++, () means the same as (void), i.e., indicates that the function accepts no +arguments.

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

+being passed. If possible, change the forward declaration of the ()-function to reflect the +arguments being passed, or change the () to (void) to indicate that no arguments +shall be passed.

 diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql index bb5a99a69bc9..c3f956f6deda 100644 --- a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql +++ b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql @@ -12,11 +12,14 @@ 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() +from FunctionCall fc, Function f + where f = fc.getTarget() and not f.isVarargs() + /* There must be a mismatch between number of call arguments and number of parameters in some + * non-implicit declaration of Function f + */ + and exists ( FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() | not fde.isImplicit() and fde.getNumberOfParameters() != fc.getNumberOfArguments()) + /* There must be no _definition_ of Function f whose number of parameters matches number of call arguments */ + and not exists ( FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() | fde.isDefinition() and fde.getNumberOfParameters() = fc.getNumberOfArguments()) +select fc + + \ No newline at end of file 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 d6dc2222be42..4bb782dfd11d 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,4 @@ -| 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 | +| test.c:7:3:7:16 | call to declared_empty | +| test.c:16:3:16:19 | call to not_yet_declared1 | +| test.c:19:3:19:29 | call to declared_empty_defined_with | +| test.c:24:3:24:29 | call to declared_empty_defined_with | 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 9589d2a9ffc9..ef81c416343a 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,7 +1,6 @@ void declared_empty(); void declared_void(void); void declared_with(int); -void declared_empty_defined_with(); void test() { declared_empty(); // GOOD @@ -9,17 +8,20 @@ void test() { declared_void(); // GOOD declared_with(1); // GOOD + declared_ellipsis(); // GOOD + declared_ellipsis(2); // GOOD + undeclared(1); // GOOD not_yet_declared1(1); // BAD not_yet_declared2(1); // GOOD - declared_empty_defined_with(); // BAD [NOT DETECTED] + declared_empty_defined_with(); // BAD declared_empty_defined_with(1); // GOOD int x; - declared_empty_defined_with(&x); // BAD [NOT DETECTED] - declared_empty_defined_with(x, x); // BAD [NOT DETECTED] + declared_empty_defined_with(&x); // GOOD + declared_empty_defined_with(x, x); // BAD } void not_yet_declared1(); From f79387e7ee46932096adfd4df25d3f8042cdfd58 Mon Sep 17 00:00:00 2001 From: Ziemowit Laski Date: Mon, 18 Mar 2019 17:23:03 -0700 Subject: [PATCH 3/5] Refine QL query by requiring that a ()-declaration be present. --- cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql index c3f956f6deda..83baeaf8a95f 100644 --- a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql +++ b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql @@ -14,6 +14,8 @@ import cpp from FunctionCall fc, Function f where f = fc.getTarget() and not f.isVarargs() + /* There must be a zero-parameter declaration */ + and exists ( FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() | fde.getNumberOfParameters() = 0) /* There must be a mismatch between number of call arguments and number of parameters in some * non-implicit declaration of Function f */ From 860abf565a9d9c2e97f40ff574df812ff630c0a4 Mon Sep 17 00:00:00 2001 From: Ziemowit Laski Date: Tue, 19 Mar 2019 12:16:08 -0700 Subject: [PATCH 4/5] Use '// GOOD' and '// BAD' annotations for query diagnostics. --- .../Memory Management/AllocaInLoop/AllocaInLoop1.cpp | 6 +++--- .../AllocaInLoop/AllocaInLoop1ms.cpp | 6 +++--- .../Memory Management/AllocaInLoop/AllocaInLoop2.c | 12 ++++++------ .../Memory Management/AllocaInLoop/AllocaInLoop3.cpp | 6 +++--- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1.cpp b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1.cpp index 54a93c511bee..c3866a9c649b 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1.cpp @@ -28,7 +28,7 @@ void foo(const struct vtype* vec, int count) { b1 = new char[w1]; } else { // Allocate the buffer on stack - b1 = (char*) alloca(w1); // [FLAG] + b1 = (char*) alloca(w1); // BAD } } memcpy(b1, v, w1); @@ -52,7 +52,7 @@ void bar(const struct vtype* vec, int count) { b1 = new char[w1]; } else { // Allocate the buffer on stack - b1 = (char*) alloca(w1); // [FLAG] + b1 = (char*) alloca(w1); // BAD } } } while (0); @@ -77,7 +77,7 @@ void baz(const struct vtype* vec, int count) { b1 = new char[w1]; } else { // Allocate the buffer on stack - b1 = (char*) alloca(w1); // [FLAG] + b1 = (char*) alloca(w1); // BAD } } memcpy(b1, v, w1); diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1ms.cpp b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1ms.cpp index 45b36161821d..9ebf4f17ba16 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1ms.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1ms.cpp @@ -25,7 +25,7 @@ void foo(const struct vtype* vec, int count) { b1 = new char[w1]; } else { // Allocate the buffer on stack - b1 = (char*) _alloca(w1); // [FLAG] + b1 = (char*) _alloca(w1); // BAD } } memcpy(b1, v, w1); @@ -49,7 +49,7 @@ void bar(const struct vtype* vec, int count) { b1 = new char[w1]; } else { // Allocate the buffer on stack - b1 = (char*) _malloca(w1); // [FLAG] + b1 = (char*) _malloca(w1); // BAD } } } while (0); @@ -76,7 +76,7 @@ void baz(const struct vtype* vec, int count) { b1 = new char[w1]; } else { // Allocate the buffer on stack - b1 = (char*) _alloca(w1); // [FLAG] + b1 = (char*) _alloca(w1); // BAD } } memcpy(b1, v, w1); diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop2.c b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop2.c index eeeafb2f17ff..60a4986441dc 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop2.c +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop2.c @@ -36,7 +36,7 @@ void foo(const struct vtype* vec, int count) { b1 = (char *)malloc(w1); } else { // Allocate the buffer on stack - b1 = (char*) alloca(w1); // [FLAG] + b1 = (char*) alloca(w1); // BAD iter = 1; } } @@ -61,7 +61,7 @@ void bar(const struct vtype* vec, int count) { b1 = (char *)malloc(w1); } else { // Allocate the buffer on stack - b1 = (char*) alloca(w1); // [DO NOT FLAG] + b1 = (char*) alloca(w1); // GOOD } } } while (0); @@ -76,7 +76,7 @@ void bar(const struct vtype* vec, int count) { // case 3: alloca called outside any loops void baz(int count) { - char *buf = (char *)alloca(32); // [DO NOT FLAG] + char *buf = (char *)alloca(32); // GOOD sprintf(buf, "Value is %d\n", count); printf("%s", buf); } @@ -94,7 +94,7 @@ void foo_ms(const struct vtype* vec, int count) { (char *)malloc(w1); } else { // Allocate the buffer on stack - b1 = (char*) _alloca(w1); // [DO NOT FLAG] + b1 = (char*) _alloca(w1); // GOOD } } memcpy(b1, v, w1); @@ -118,7 +118,7 @@ void bar_ms(const struct vtype* vec, int count) { b1 = (char *)malloc(w1); } else { // Allocate the buffer on stack - b1 = (char*) _malloca(w1); // [DO NOT FLAG] + b1 = (char*) _malloca(w1); // GOOD } } } while (0); @@ -145,7 +145,7 @@ void baz_ms(const struct vtype* vec, int count) { b1 = (char *)malloc(w1); } else { // Allocate the buffer on stack - b1 = (char*) _alloca(w1); // [DO NOT FLAG] + b1 = (char*) _alloca(w1); // GOOD } } memcpy(b1, v, w1); diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop3.cpp b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop3.cpp index 60c02958d106..8a14c58355c0 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop3.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop3.cpp @@ -15,7 +15,7 @@ void *__builtin_alloca(int sz); // case 1: a GNU c/c++ lambda with an alloca in it char *foo(int count) { char *buf = ({ - char *b = (char *)alloca(32); // [DO NOT FLAG] + char *b = (char *)alloca(32); // GOOD sprintf(b, "Value is %d\n", count); b; }); @@ -28,7 +28,7 @@ char *bar(int count) { char *buf; do { buf = ({ - char *b = (char *)alloca(32); // [DO NOT FLAG] + char *b = (char *)alloca(32); // GOOD sprintf(b, "Value is %d\n", count); b; }); @@ -42,7 +42,7 @@ char *baz(int count) { char *buf; do { buf = ({ - char *b = (char *)alloca(32); // [FLAG] + char *b = (char *)alloca(32); // BAD sprintf(b, "Value is %d\n", count); b; }); From 93b76a3558380187a14ec9e15d9896d4ef1d7a09 Mon Sep 17 00:00:00 2001 From: Ziemowit Laski Date: Tue, 19 Mar 2019 14:30:05 -0700 Subject: [PATCH 5/5] [CPP-340] Re-work QL query; treat undeclared C functions the same way as ()-declared functions. --- cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql | 10 +++++----- .../Likely Typos/FutileParams/FutileParams.expected | 10 ++++++---- .../Likely Bugs/Likely Typos/FutileParams/test.c | 6 ++---- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql index 83baeaf8a95f..c21afaf4ac9a 100644 --- a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql +++ b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql @@ -17,11 +17,11 @@ from FunctionCall fc, Function f /* There must be a zero-parameter declaration */ and exists ( FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() | fde.getNumberOfParameters() = 0) /* There must be a mismatch between number of call arguments and number of parameters in some - * non-implicit declaration of Function f + * declaration of Function f */ - and exists ( FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() | not fde.isImplicit() and fde.getNumberOfParameters() != fc.getNumberOfArguments()) - /* There must be no _definition_ of Function f whose number of parameters matches number of call arguments */ - and not exists ( FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() | fde.isDefinition() and fde.getNumberOfParameters() = fc.getNumberOfArguments()) -select fc + and exists ( FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() | fde.getNumberOfParameters() != fc.getNumberOfArguments()) + /* There must be no actual declaration of Function f whose number of parameters matches number of call arguments */ + and not exists ( FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() | not fde.isImplicit() and fde.getNumberOfParameters() = fc.getNumberOfArguments()) +select fc, "This call has arguments, but $@ is not declared with any parameters.", f, f.toString() \ No newline at end of file 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 4bb782dfd11d..0021164c8614 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,4 +1,6 @@ -| test.c:7:3:7:16 | call to declared_empty | -| test.c:16:3:16:19 | call to not_yet_declared1 | -| test.c:19:3:19:29 | call to declared_empty_defined_with | -| test.c:24:3:24:29 | call to declared_empty_defined_with | +| test.c:7:3:7: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:12:3:12:12 | call to undeclared | This call has arguments, but $@ is not declared with any parameters. | test.c:11:3:11:3 | undeclared | undeclared | +| 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 | +| test.c:17:3:17:29 | call to declared_empty_defined_with | This call has arguments, but $@ is not declared with any parameters. | test.c:27:6:27:32 | declared_empty_defined_with | declared_empty_defined_with | +| test.c:22:3:22:29 | call to declared_empty_defined_with | This call has arguments, but $@ is not declared with any parameters. | test.c:27:6:27:32 | declared_empty_defined_with | declared_empty_defined_with | 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 ef81c416343a..698644c5c1cf 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 @@ -8,10 +8,8 @@ void test() { declared_void(); // GOOD declared_with(1); // GOOD - declared_ellipsis(); // GOOD - declared_ellipsis(2); // GOOD - - undeclared(1); // GOOD + undeclared(); // GOOD + undeclared(1); // BAD not_yet_declared1(1); // BAD not_yet_declared2(1); // GOOD