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..c21afaf4ac9a 100644 --- a/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql +++ b/cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql @@ -12,11 +12,16 @@ 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 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 + * declaration of Function f + */ + 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/src/Likely Bugs/Memory Management/AllocaInLoop.ql b/cpp/ql/src/Likely Bugs/Memory Management/AllocaInLoop.ql index e4a3163d4d1a..635a1013b716 100644 --- a/cpp/ql/src/Likely Bugs/Memory Management/AllocaInLoop.ql +++ b/cpp/ql/src/Likely Bugs/Memory Management/AllocaInLoop.ql @@ -8,6 +8,7 @@ * correctness * external/cwe/cwe-770 */ + import cpp Loop getAnEnclosingLoopOfExpr(Expr e) { @@ -21,7 +22,15 @@ Loop getAnEnclosingLoopOfStmt(Stmt s) { } from Loop l, FunctionCall fc -where getAnEnclosingLoopOfExpr(fc) = l - and fc.getTarget().getName() = "__builtin_alloca" - and not l.(DoStmt).getCondition().getValue() = "0" -select fc, "Stack allocation is inside a $@ and could lead to overflow.", l, l.toString() +where + getAnEnclosingLoopOfExpr(fc) = l and + ( + fc.getTarget().getName() = "__builtin_alloca" + or + ( + (fc.getTarget().getName() = "_alloca" or fc.getTarget().getName() = "_malloca") and + fc.getTarget().getADeclarationEntry().getFile().getBaseName() = "malloc.h" + ) + ) and + not l.(DoStmt).getCondition().getValue() = "0" +select fc, "Stack allocation is inside a $@ and could lead to stack overflow.", l, l.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 index d6dc2222be42..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,3 +1,6 @@ -| 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: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 9589d2a9ffc9..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 @@ -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,18 @@ void test() { declared_void(); // GOOD declared_with(1); // GOOD - undeclared(1); // GOOD + undeclared(); // GOOD + undeclared(1); // BAD 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(); diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop.expected b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop.expected new file mode 100644 index 000000000000..df4910ac5aaf --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop.expected @@ -0,0 +1,8 @@ +| AllocaInLoop1.cpp:31:18:31:23 | call to __builtin_alloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop1.cpp:22:2:39:2 | for(...;...;...) ... | for(...;...;...) ... | +| AllocaInLoop1.cpp:55:19:55:24 | call to __builtin_alloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop1.cpp:45:2:64:2 | for(...;...;...) ... | for(...;...;...) ... | +| AllocaInLoop1.cpp:80:19:80:24 | call to __builtin_alloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop1.cpp:71:3:88:3 | for(...;...;...) ... | for(...;...;...) ... | +| AllocaInLoop1ms.cpp:28:18:28:24 | call to _alloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop1ms.cpp:19:2:36:2 | for(...;...;...) ... | for(...;...;...) ... | +| AllocaInLoop1ms.cpp:52:19:52:26 | call to _malloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop1ms.cpp:42:2:63:2 | for(...;...;...) ... | for(...;...;...) ... | +| AllocaInLoop1ms.cpp:79:19:79:25 | call to _alloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop1ms.cpp:70:3:87:3 | for(...;...;...) ... | for(...;...;...) ... | +| AllocaInLoop2.c:39:30:39:35 | call to __builtin_alloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop2.c:29:5:48:19 | do (...) ... | do (...) ... | +| AllocaInLoop3.cpp:45:23:45:28 | call to __builtin_alloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop3.cpp:43:2:49:19 | do (...) ... | do (...) ... | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop.qlref b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop.qlref new file mode 100644 index 000000000000..d5227c40ee4c --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop.qlref @@ -0,0 +1 @@ +Likely Bugs/Memory Management/AllocaInLoop.ql 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 new file mode 100644 index 000000000000..c3866a9c649b --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1.cpp @@ -0,0 +1,90 @@ +// semmle-extractor-options: --clang +struct vtype { + int i1, i2; +}; +extern int w1, w2; + +#ifdef _MSC_VER +#define restrict __restrict +#else +#define restrict __restrict__ +#endif + +void *__builtin_alloca(int sz); +#define alloca __builtin_alloca +typedef unsigned long size_t; + +int printf(const char *restrict format, ...); +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) { + for (int i = 0; i < count; i++) { + const vtype* v = vec + i; + char *b1 = 0; + if (b1 == nullptr) { + if (w1 > w2) { + // Allocate the buffer on heap + b1 = new char[w1]; + } else { + // Allocate the buffer on stack + b1 = (char*) alloca(w1); // BAD + } + } + memcpy(b1, v, w1); + printf("%s\n", b1); + if (w1 > w2) { + delete b1; + } + } +} + +// case 2: alloca contained in a do-while(0) that is in turn contained +// in an unbounded loop +void bar(const struct vtype* vec, int count) { + for (int i = 0; i < count; i++) { + const vtype* v = vec + i; + char *b1 = 0; + do { + if (b1 == nullptr) { + if (w1 > w2) { + // Allocate the buffer on heap + b1 = new char[w1]; + } else { + // Allocate the buffer on stack + b1 = (char*) alloca(w1); // BAD + } + } + } while (0); + memcpy(b1, v, w1); + printf("%s\n", b1); + if (w1 > w2) { + delete b1; + } + } +} + +// case 3: alloca contained in an unbounded loop that is in turn contained +// in a do-while(0) +void baz(const struct vtype* vec, int count) { + do { + for (int i = 0; i < count; i++) { + const vtype* v = vec + i; + char *b1 = 0; + if (b1 == nullptr) { + if (w1 > w2) { + // Allocate the buffer on heap + b1 = new char[w1]; + } else { + // Allocate the buffer on stack + b1 = (char*) alloca(w1); // BAD + } + } + memcpy(b1, v, w1); + printf("%s\n", b1); + if (w1 > w2) { + delete b1; + } + } + } while (0); +} 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 new file mode 100644 index 000000000000..9ebf4f17ba16 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1ms.cpp @@ -0,0 +1,89 @@ +// semmle-extractor-options: --clang +#include "malloc.h" +struct vtype { + int i1, i2; +}; +extern int w1, w2; + +#ifdef _MSC_VER +#define restrict __restrict +#else +#define restrict __restrict__ +#endif + +int printf(const char *restrict format, ...); +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) { + for (int i = 0; i < count; i++) { + const vtype* v = vec + i; + char *b1 = 0; + if (b1 == nullptr) { + if (w1 > w2) { + // Allocate the buffer on heap + b1 = new char[w1]; + } else { + // Allocate the buffer on stack + b1 = (char*) _alloca(w1); // BAD + } + } + memcpy(b1, v, w1); + printf("%s\n", b1); + if (w1 > w2) { + delete b1; + } + } +} + +// case 2: _malloca contained in a do-while(0) that is in turn contained +// in an unbounded loop +void bar(const struct vtype* vec, int count) { + for (int i = 0; i < count; i++) { + const vtype* v = vec + i; + char *b1 = 0; + do { + if (b1 == nullptr) { + if (w1 > w2) { + // Allocate the buffer on heap + b1 = new char[w1]; + } else { + // Allocate the buffer on stack + b1 = (char*) _malloca(w1); // BAD + } + } + } while (0); + memcpy(b1, v, w1); + printf("%s\n", b1); + if (w1 > w2) { + delete b1; + } else { + _freea(b1); + } + } +} + +// case 3: _alloca contained in an unbounded loop that is in turn contained +// in a do-while(0) +void baz(const struct vtype* vec, int count) { + do { + for (int i = 0; i < count; i++) { + const vtype* v = vec + i; + char *b1 = 0; + if (b1 == nullptr) { + if (w1 > w2) { + // Allocate the buffer on heap + b1 = new char[w1]; + } else { + // Allocate the buffer on stack + b1 = (char*) _alloca(w1); // BAD + } + } + memcpy(b1, v, w1); + printf("%s\n", b1); + if (w1 > w2) { + delete b1; + } + } + } while (0); +} 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 new file mode 100644 index 000000000000..60a4986441dc --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop2.c @@ -0,0 +1,158 @@ +// semmle-extractor-options: --clang +int printf(const char *restrict format, ...); +int sprintf(char *restrict buf, const char *restrict format, ...); +typedef unsigned long size_t; +void *memcpy(void *restrict s1, const void *restrict s2, size_t n); +void *malloc(size_t sz); +void free(void *ptr); + +struct vtype { int i1, i2; }; +extern int w1, w2; + +void *_builtin_alloca(int sz); +#define alloca __builtin_alloca + +// We forward-declare the Microsoft routines +//_alloca and _malloca here. Since they do not +// originate from the header, they +// should not be flagged by our queries +void *_alloca(size_t sz); +void *_malloca(size_t sz); +void _freea(void *ptr); + +#define NULL (void *)0 + +// case 1: alloca called within a provably infinite loop +void foo(const struct vtype* vec, int count) { + char iter; + + do { + const struct vtype* v = vec + 2; + char *b1 = 0; + iter = 0; + if (b1 == NULL) { + if (w1 > w2) { + // Allocate the buffer on heap + b1 = (char *)malloc(w1); + } else { + // Allocate the buffer on stack + b1 = (char*) alloca(w1); // BAD + iter = 1; + } + } + memcpy(b1, v, w1); + printf("%s\n", b1); + if (w1 > w2) { + free(b1); + } + } while (iter); +} + +// case 2: alloca called within nested do-while(0) loops +void bar(const struct vtype* vec, int count) { + + do { + const struct vtype* v = vec + 2; + char *b1 = 0; + do { + if (b1 == NULL) { + if (w1 > w2) { + // Allocate the buffer on heap + b1 = (char *)malloc(w1); + } else { + // Allocate the buffer on stack + b1 = (char*) alloca(w1); // GOOD + } + } + } while (0); + memcpy(b1, v, w1); + printf("%s\n", b1); + if (w1 > w2) { + free(b1); + } + } while (0); +} + +// case 3: alloca called outside any loops +void baz(int count) { + + char *buf = (char *)alloca(32); // GOOD + sprintf(buf, "Value is %d\n", count); + printf("%s", buf); +} + +////// Negative Microsoft test cases + +// case 4: _alloca directly contained in an unbounded loop +void foo_ms(const struct vtype* vec, int count) { + for (int i = 0; i < count; i++) { + const struct vtype* v = vec + i; + char *b1 = 0; + if (b1 == NULL) { + if (w1 > w2) { + // Allocate the buffer on heap + (char *)malloc(w1); + } else { + // Allocate the buffer on stack + b1 = (char*) _alloca(w1); // GOOD + } + } + memcpy(b1, v, w1); + printf("%s\n", b1); + if (w1 > w2) { + free(b1); + } + } +} + +// case 5: _malloca contained in a do-while(0) that is in turn contained +// in an unbounded loop +void bar_ms(const struct vtype* vec, int count) { + for (int i = 0; i < count; i++) { + const struct vtype* v = vec + i; + char *b1 = 0; + do { + if (b1 == NULL) { + if (w1 > w2) { + // Allocate the buffer on heap + b1 = (char *)malloc(w1); + } else { + // Allocate the buffer on stack + b1 = (char*) _malloca(w1); // GOOD + } + } + } while (0); + memcpy(b1, v, w1); + printf("%s\n", b1); + if (w1 > w2) { + free(b1); + } else { + _freea(b1); + } + } +} + +// case 6: _alloca contained in an unbounded loop that is in turn contained +// in a do-while(0) +void baz_ms(const struct vtype* vec, int count) { + do { + for (int i = 0; i < count; i++) { + const struct vtype* v = vec + i; + char *b1 = 0; + if (b1 == NULL) { + if (w1 > w2) { + // Allocate the buffer on heap + b1 = (char *)malloc(w1); + } else { + // Allocate the buffer on stack + b1 = (char*) _alloca(w1); // GOOD + } + } + memcpy(b1, v, w1); + printf("%s\n", b1); + if (w1 > w2) { + free(b1); + } + } + } while (0); +} 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 new file mode 100644 index 000000000000..8a14c58355c0 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop3.cpp @@ -0,0 +1,51 @@ +// semmle-extractor-options: --clang + +#ifdef _MSC_VER +#define restrict __restrict +#else +#define restrict __restrict__ +#endif + +int sprintf(char *restrict buf, const char *restrict format, ...); +char * strdup(const char *restrict str1); + +void *__builtin_alloca(int sz); +#define alloca __builtin_alloca + +// case 1: a GNU c/c++ lambda with an alloca in it +char *foo(int count) { + char *buf = ({ + char *b = (char *)alloca(32); // GOOD + sprintf(b, "Value is %d\n", count); + b; + }); + return strdup(buf); +} + +// case 1: a GNU expression statement with an alloca in it +// nested inside a do-while(0) +char *bar(int count) { + char *buf; + do { + buf = ({ + char *b = (char *)alloca(32); // GOOD + sprintf(b, "Value is %d\n", count); + b; + }); + } while (0); + return strdup(buf); +} + +// case 2: a GNU expression statement with an alloca in it +// nested inside an unbounded loop +char *baz(int count) { + char *buf; + do { + buf = ({ + char *b = (char *)alloca(32); // BAD + sprintf(b, "Value is %d\n", count); + b; + }); + } while (count++); + return strdup(buf); +} diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/malloc.h b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/malloc.h new file mode 100644 index 000000000000..24344f4719f1 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/malloc.h @@ -0,0 +1,7 @@ +typedef unsigned long size_t; +// These are Microsoft routines for stack allocation +// but should only be treated as such if they are declared +// in the header (i.e., this file) +void *_alloca(size_t sz); +void *_malloca(size_t sz); +void _freea(void *ptr);