Merge pull request #2622 from MathiasVP/implicit-function-declaration

C++: Add 'implicit function declaration' query

change-notes/1.24/analysis-cpp.md

@@ -8,6 +8,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
+| Implicit function declarations (`cpp/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.ql`) | correctness, maintainability | This query finds calls to undeclared functions that are compiled by a C compiler. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 

cpp/config/suites/c/correctness

@@ -18,6 +18,7 @@
 + semmlecode-cpp-queries/Likely Bugs/Likely Typos/ExprHasNoEffect.ql: /Correctness/Common Errors
 + semmlecode-cpp-queries/Likely Bugs/Underspecified Functions/TooFewArguments.ql: /Correctness/Common Errors
 + semmlecode-cpp-queries/Likely Bugs/Underspecified Functions/TooManyArguments.ql: /Correctness/Common Errors
++ semmlecode-cpp-queries/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.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

cpp/config/suites/cpp/correctness

@@ -19,6 +19,7 @@
 + semmlecode-cpp-queries/Likely Bugs/Likely Typos/ExprHasNoEffect.ql: /Correctness/Common Errors
 + semmlecode-cpp-queries/Likely Bugs/Underspecified Functions/TooFewArguments.ql: /Correctness/Common Errors
 + semmlecode-cpp-queries/Likely Bugs/Underspecified Functions/TooManyArguments.ql: /Correctness/Common Errors
++ semmlecode-cpp-queries/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.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

cpp/ql/src/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.c

@@ -0,0 +1,8 @@
+/* '#include <stdlib.h>' was forgotton */
+
+int main(void) {
+	/* 'int malloc()' assumed */
+	unsigned char *p = malloc(100);
+	*p = 'a';
+	return 0;
+}

cpp/ql/src/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.qhelp

@@ -0,0 +1,29 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>A function is called without a prior function declaration or definition.
+When this happens, the compiler generates an implicit declaration of the function,
+specifying an integer return type and no parameters.
+If the implicit declaration does not match the true signature of the function, the
+function may behave unpredictably.</p>
+
+<p>This may indicate a misspelled function name, or that the required header containing
+the function declaration has not been included.</p>
+
+</overview>
+<recommendation>
+<p>Provide an explicit declaration of the function before invoking it.</p>
+
+</recommendation>
+<example><sample src="ImplicitFunctionDeclaration.c" />
+
+</example>
+
+<references>
+<li>SEI CERT C Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/c/DCL31-C.+Declare+identifiers+before+using+them">DCL31-C. Declare identifiers before using them</a></li>
+</references>
+</qhelp>

cpp/ql/src/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.ql

@@ -0,0 +1,48 @@
+/**
+ * @name Implicit function declaration
+ * @description An implicitly declared function is assumed to take no
+ * arguments and return an integer. If this assumption does not hold, it
+ * may lead to unpredictable behavior.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id cpp/implicit-function-declaration
+ * @tags correctness
+ *       maintainability
+ */
+
+import cpp
+import MistypedFunctionArguments
+import TooFewArguments
+import TooManyArguments
+import semmle.code.cpp.commons.Exclusions
+
+predicate locInfo(Locatable e, File file, int line, int col) {
+  e.getFile() = file and
+  e.getLocation().getStartLine() = line and
+  e.getLocation().getStartColumn() = col
+}
+
+predicate sameLocation(FunctionDeclarationEntry fde, FunctionCall fc) {
+  exists(File file, int line, int col |
+    locInfo(fde, file, line, col) and
+    locInfo(fc, file, line, col)
+  )
+}
+
+predicate isCompiledAsC(File f) {
+  f.compiledAsC()
+  or
+  exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f)
+}
+
+from FunctionDeclarationEntry fdeIm, FunctionCall fc
+where
+  isCompiledAsC(fdeIm.getFile()) and
+  not isFromMacroDefinition(fc) and
+  fdeIm.isImplicit() and
+  sameLocation(fdeIm, fc) and
+  not mistypedFunctionArguments(fc, _, _) and
+  not tooFewArguments(fc, _) and
+  not tooManyArguments(fc, _)
+select fc, "Function call implicitly declares '" + fdeIm.getName() + "'."

cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.ql

@@ -12,95 +12,10 @@
  */
 
 import cpp
-
+import MistypedFunctionArguments
-predicate arithTypesMatch(Type arg, Type parm) {
-  arg = parm
-  or
-  arg.getSize() = parm.getSize() and
-  (
-    arg instanceof IntegralOrEnumType and
-    parm instanceof IntegralOrEnumType
-    or
-    arg instanceof FloatingPointType and
-    parm instanceof FloatingPointType
-  )
-}
-
-pragma[inline]
-predicate nestedPointerArgTypeMayBeUsed(Type arg, Type parm) {
-  // arithmetic types
-  arithTypesMatch(arg, parm)
-  or
-  // conversion to/from pointers to void is allowed
-  arg instanceof VoidType
-  or
-  parm instanceof VoidType
-}
-
-pragma[inline]
-predicate pointerArgTypeMayBeUsed(Type arg, Type parm) {
-  nestedPointerArgTypeMayBeUsed(arg, parm)
-  or
-  // nested pointers
-  nestedPointerArgTypeMayBeUsed(arg.(PointerType).getBaseType().getUnspecifiedType(),
-    parm.(PointerType).getBaseType().getUnspecifiedType())
-  or
-  nestedPointerArgTypeMayBeUsed(arg.(ArrayType).getBaseType().getUnspecifiedType(),
-    parm.(PointerType).getBaseType().getUnspecifiedType())
-}
-
-pragma[inline]
-predicate argTypeMayBeUsed(Type arg, Type parm) {
-  // arithmetic types
-  arithTypesMatch(arg, parm)
-  or
-  // pointers to compatible types
-  pointerArgTypeMayBeUsed(arg.(PointerType).getBaseType().getUnspecifiedType(),
-    parm.(PointerType).getBaseType().getUnspecifiedType())
-  or
-  pointerArgTypeMayBeUsed(arg.(ArrayType).getBaseType().getUnspecifiedType(),
-    parm.(PointerType).getBaseType().getUnspecifiedType())
-  or
-  // C11 arrays
-  pointerArgTypeMayBeUsed(arg.(PointerType).getBaseType().getUnspecifiedType(),
-    parm.(ArrayType).getBaseType().getUnspecifiedType())
-  or
-  pointerArgTypeMayBeUsed(arg.(ArrayType).getBaseType().getUnspecifiedType(),
-    parm.(ArrayType).getBaseType().getUnspecifiedType())
-}
-
-// This predicate holds whenever expression `arg` may be used to initialize
-// function parameter `parm` without need for run-time conversion.
-pragma[inline]
-predicate argMayBeUsed(Expr arg, Parameter parm) {
-  argTypeMayBeUsed(arg.getFullyConverted().getUnspecifiedType(), parm.getUnspecifiedType())
-}
-
-// True if function was ()-declared, but not (void)-declared or K&R-defined
-predicate hasZeroParamDecl(Function f) {
-  exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |
-    not fde.hasVoidParamList() and fde.getNumberOfParameters() = 0 and not fde.isDefinition()
-  )
-}
-
-// True if this file (or header) was compiled as a C file
-predicate isCompiledAsC(File f) {
-  f.compiledAsC()
-  or
-  exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f)
-}
 
 from FunctionCall fc, Function f, Parameter p
-where
+where mistypedFunctionArguments(fc, f, p)
-  f = fc.getTarget() and
-  p = f.getAParameter() and
-  hasZeroParamDecl(f) and
-  isCompiledAsC(f.getFile()) and
-  not f.isVarargs() and
-  not f instanceof BuiltInFunction and
-  p.getIndex() < fc.getNumberOfArguments() and
-  // Parameter p and its corresponding call argument must have mismatched types
-  not argMayBeUsed(fc.getArgument(p.getIndex()), p)
 select fc, "Calling $@: argument $@ of type $@ is incompatible with parameter $@.", f, f.toString(),
   fc.getArgument(p.getIndex()) as arg, arg.toString(),
   arg.getExplicitlyConverted().getUnspecifiedType() as atype, atype.toString(), p, p.getTypedName()

cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.qll

@@ -0,0 +1,96 @@
+/**
+ * Provides the implementation of the MistypedFunctionArguments query. The
+ * query is implemented as a library, so that we can avoid producing
+ * duplicate results in other similar queries.
+ */
+
+import cpp
+
+private predicate arithTypesMatch(Type arg, Type parm) {
+  arg = parm
+  or
+  arg.getSize() = parm.getSize() and
+  (
+    arg instanceof IntegralOrEnumType and
+    parm instanceof IntegralOrEnumType
+    or
+    arg instanceof FloatingPointType and
+    parm instanceof FloatingPointType
+  )
+}
+
+pragma[inline]
+private predicate nestedPointerArgTypeMayBeUsed(Type arg, Type parm) {
+  // arithmetic types
+  arithTypesMatch(arg, parm)
+  or
+  // conversion to/from pointers to void is allowed
+  arg instanceof VoidType
+  or
+  parm instanceof VoidType
+}
+
+pragma[inline]
+private predicate pointerArgTypeMayBeUsed(Type arg, Type parm) {
+  nestedPointerArgTypeMayBeUsed(arg, parm)
+  or
+  // nested pointers
+  nestedPointerArgTypeMayBeUsed(arg.(PointerType).getBaseType().getUnspecifiedType(),
+    parm.(PointerType).getBaseType().getUnspecifiedType())
+  or
+  nestedPointerArgTypeMayBeUsed(arg.(ArrayType).getBaseType().getUnspecifiedType(),
+    parm.(PointerType).getBaseType().getUnspecifiedType())
+}
+
+pragma[inline]
+private predicate argTypeMayBeUsed(Type arg, Type parm) {
+  // arithmetic types
+  arithTypesMatch(arg, parm)
+  or
+  // pointers to compatible types
+  pointerArgTypeMayBeUsed(arg.(PointerType).getBaseType().getUnspecifiedType(),
+    parm.(PointerType).getBaseType().getUnspecifiedType())
+  or
+  pointerArgTypeMayBeUsed(arg.(ArrayType).getBaseType().getUnspecifiedType(),
+    parm.(PointerType).getBaseType().getUnspecifiedType())
+  or
+  // C11 arrays
+  pointerArgTypeMayBeUsed(arg.(PointerType).getBaseType().getUnspecifiedType(),
+    parm.(ArrayType).getBaseType().getUnspecifiedType())
+  or
+  pointerArgTypeMayBeUsed(arg.(ArrayType).getBaseType().getUnspecifiedType(),
+    parm.(ArrayType).getBaseType().getUnspecifiedType())
+}
+
+// This predicate holds whenever expression `arg` may be used to initialize
+// function parameter `parm` without need for run-time conversion.
+pragma[inline]
+private predicate argMayBeUsed(Expr arg, Parameter parm) {
+  argTypeMayBeUsed(arg.getFullyConverted().getUnspecifiedType(), parm.getUnspecifiedType())
+}
+
+// True if function was ()-declared, but not (void)-declared or K&R-defined
+private predicate hasZeroParamDecl(Function f) {
+  exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |
+    not fde.hasVoidParamList() and fde.getNumberOfParameters() = 0 and not fde.isDefinition()
+  )
+}
+
+// True if this file (or header) was compiled as a C file
+private predicate isCompiledAsC(File f) {
+  f.compiledAsC()
+  or
+  exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f)
+}
+
+predicate mistypedFunctionArguments(FunctionCall fc, Function f, Parameter p) {
+  f = fc.getTarget() and
+  p = f.getAParameter() and
+  hasZeroParamDecl(f) and
+  isCompiledAsC(f.getFile()) and
+  not f.isVarargs() and
+  not f instanceof BuiltInFunction and
+  p.getIndex() < fc.getNumberOfArguments() and
+  // Parameter p and its corresponding call argument must have mismatched types
+  not argMayBeUsed(fc.getArgument(p.getIndex()), p)
+}

cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.ql

@@ -15,31 +15,8 @@
  */
 
 import cpp
-
+import TooFewArguments
-// True if function was ()-declared, but not (void)-declared or K&R-defined
-predicate hasZeroParamDecl(Function f) {
-  exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |
-    not fde.hasVoidParamList() and fde.getNumberOfParameters() = 0 and not fde.isDefinition()
-  )
-}
-
-// True if this file (or header) was compiled as a C file
-predicate isCompiledAsC(File f) {
-  f.compiledAsC()
-  or
-  exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f)
-}
 
 from FunctionCall fc, Function f
-where
+where tooFewArguments(fc, f)
-  f = fc.getTarget() and
-  not f.isVarargs() and
-  not f instanceof BuiltInFunction and
-  hasZeroParamDecl(f) and
-  isCompiledAsC(f.getFile()) and
-  // There is an explicit declaration of the function whose parameter count is larger
-  // than the number of call arguments
-  exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |
-    fde.getNumberOfParameters() > fc.getNumberOfArguments()
-  )
 select fc, "This call has fewer arguments than required by $@.", f, f.toString()

cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.qll

@@ -0,0 +1,34 @@
+/**
+ * Provides the implementation of the TooFewArguments query. The
+ * query is implemented as a library, so that we can avoid producing
+ * duplicate results in other similar queries.
+ */
+
+import cpp
+
+// True if function was ()-declared, but not (void)-declared or K&R-defined
+private predicate hasZeroParamDecl(Function f) {
+  exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |
+    not fde.hasVoidParamList() and fde.getNumberOfParameters() = 0 and not fde.isDefinition()
+  )
+}
+
+// True if this file (or header) was compiled as a C file
+private predicate isCompiledAsC(File f) {
+  f.compiledAsC()
+  or
+  exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f)
+}
+
+predicate tooFewArguments(FunctionCall fc, Function f) {
+  f = fc.getTarget() and
+  not f.isVarargs() and
+  not f instanceof BuiltInFunction and
+  hasZeroParamDecl(f) and
+  isCompiledAsC(f.getFile()) and
+  // There is an explicit declaration of the function whose parameter count is larger
+  // than the number of call arguments
+  exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |
+    fde.getNumberOfParameters() > fc.getNumberOfArguments()
+  )
+}

cpp/ql/src/Likely Bugs/Underspecified Functions/TooManyArguments.ql

@@ -12,35 +12,8 @@
  */
 
 import cpp
-
+import TooManyArguments
-// True if function was ()-declared, but not (void)-declared or K&R-defined
-// or implicitly declared (i.e., lacking a prototype)
-predicate hasZeroParamDecl(Function f) {
-  exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |
-    not fde.isImplicit() and
-    not fde.hasVoidParamList() and
-    fde.getNumberOfParameters() = 0 and
-    not fde.isDefinition()
-  )
-}
-
-// True if this file (or header) was compiled as a C file
-predicate isCompiledAsC(File f) {
-  f.compiledAsC()
-  or
-  exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f)
-}
 
 from FunctionCall fc, Function f
-where
+where tooManyArguments(fc, f)
-  f = fc.getTarget() and
-  not f.isVarargs() and
-  hasZeroParamDecl(f) and
-  isCompiledAsC(f.getFile()) and
-  exists(f.getBlock()) and
-  // There must not exist a declaration with the number of parameters
-  // at least as large as the number of call arguments
-  not exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |
-    fde.getNumberOfParameters() >= fc.getNumberOfArguments()
-  )
 select fc, "This call has more arguments than required by $@.", f, f.toString()

cpp/ql/src/Likely Bugs/Underspecified Functions/TooManyArguments.qll

@@ -0,0 +1,38 @@
+/**
+ * Provides the implementation of the TooManyArguments query. The
+ * query is implemented as a library, so that we can avoid producing
+ * duplicate results in other similar queries.
+ */
+
+import cpp
+
+// True if function was ()-declared, but not (void)-declared or K&R-defined
+// or implicitly declared (i.e., lacking a prototype)
+private predicate hasZeroParamDecl(Function f) {
+  exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |
+    not fde.isImplicit() and
+    not fde.hasVoidParamList() and
+    fde.getNumberOfParameters() = 0 and
+    not fde.isDefinition()
+  )
+}
+
+// True if this file (or header) was compiled as a C file
+private predicate isCompiledAsC(File f) {
+  f.compiledAsC()
+  or
+  exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f)
+}
+
+predicate tooManyArguments(FunctionCall fc, Function f) {
+  f = fc.getTarget() and
+  not f.isVarargs() and
+  hasZeroParamDecl(f) and
+  isCompiledAsC(f.getFile()) and
+  exists(f.getBlock()) and
+  // There must not exist a declaration with the number of parameters
+  // at least as large as the number of call arguments
+  not exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |
+    fde.getNumberOfParameters() >= fc.getNumberOfArguments()
+  )
+}

cpp/ql/test/query-tests/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.expected

@@ -0,0 +1,9 @@
+| test.c:28:3:28:12 | call to undeclared | Function call implicitly declares 'undeclared'. |
+| test.c:31:3:31:19 | call to not_yet_declared1 | Function call implicitly declares 'not_yet_declared1'. |
+| test.c:32:3:32:19 | call to not_yet_declared2 | Function call implicitly declares 'not_yet_declared2'. |
+| test.c:43:3:43:27 | call to not_declared_defined_with | Function call implicitly declares 'not_declared_defined_with'. |
+| test.c:54:3:54:21 | call to defined_with_double | Function call implicitly declares 'defined_with_double'. |
+| test.c:66:3:66:22 | call to defined_with_ptr_ptr | Function call implicitly declares 'defined_with_ptr_ptr'. |
+| test.c:68:3:68:22 | call to defined_with_ptr_arr | Function call implicitly declares 'defined_with_ptr_arr'. |
+| test.c:132:3:132:22 | call to implicit_declaration | Function call implicitly declares 'implicit_declaration'. |
+| test.c:133:3:133:30 | call to implicit_declaration_k_and_r | Function call implicitly declares 'implicit_declaration_k_and_r'. |

cpp/ql/test/query-tests/Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.qlref

@@ -0,0 +1 @@
+Likely Bugs/Underspecified Functions/ImplicitFunctionDeclaration.ql

cpp/ql/test/query-tests/Likely Bugs/Underspecified Functions/test.c

@@ -25,11 +25,11 @@ void test(int *argv[]) {
   declared_void(); // GOOD
   declared_with(1); // GOOD
   
-  undeclared();  // GOOD
+  undeclared();  // BAD (GOOD for everything except cpp/implicit-function-declaration)
   undeclared(1); // GOOD
   
-  not_yet_declared1(1); // GOOD
+  not_yet_declared1(1); // BAD (GOOD for everything except for cpp/implicit-function-declaration)
-  not_yet_declared2(1); // GOOD
+  not_yet_declared2(1); // BAD (GOOD for everything except for cpp/implicit-function-declaration)
   not_yet_declared2(ca); // BAD
   not_yet_declared2(); // BAD
 
@@ -40,7 +40,7 @@ void test(int *argv[]) {
   declared_empty_defined_with(&x); // BAD
   declared_empty_defined_with(3, &x); // BAD
 
-  not_declared_defined_with(-1, 0, 2U); // GOOD
+  not_declared_defined_with(-1, 0, 2U); // BAD (GOOD for everything except for cpp/implicit-function-declaration)
   not_declared_defined_with(4LL, 0, 2.5e9f); // BAD
 
   declared_with_pointers(pv, ca); // GOOD
@@ -51,7 +51,7 @@ void test(int *argv[]) {
   defined_with_float(2.f);  // BAD
   defined_with_float(2.0);  // BAD
   
-  defined_with_double(2.f); // GOOD
+  defined_with_double(2.f); // BAD (GOOD for everything except for cpp/implicit-function-declaration)
   defined_with_double('c');  // BAD
   
   defined_with_long_long('c');  // BAD
@@ -63,9 +63,9 @@ void test(int *argv[]) {
   k_and_r_func(2.5, &s);  // GOOD
   
   int (*parameterName)[2];
-  defined_with_ptr_ptr(parameterName); // GOOD
+  defined_with_ptr_ptr(parameterName); // // BAD (GOOD for everything except for cpp/implicit-function-declaration)
   defined_with_ptr_ptr(argv);   // GOOD
-  defined_with_ptr_arr(parameterName); // GOOD
+  defined_with_ptr_arr(parameterName); // // BAD (GOOD for everything except for cpp/implicit-function-declaration)
   defined_with_ptr_arr(argv);   // GOOD
 
   declared_and_defined_empty(); // GOOD
@@ -124,3 +124,15 @@ int call_k_and_r(int i) {
 int will_be_k_and_r(val)
   int val;
 { return val + 1; }
+
+extern int extern_definition(double, double*);
+
+void test_implicit_function_declaration(int x, double d) {
+  int y;
+  implicit_declaration(1, 2); // BAD
+  implicit_declaration_k_and_r(1, 2); // BAD
+
+  implicit_declaration(1, 2); // GOOD (no longer an implicit declaration)
+
+  y = extern_definition(3.0f, &d); // GOOD
+}

cpp/ql/test/query-tests/Likely Bugs/Underspecified Functions/test2.c

@@ -0,0 +1,6 @@
+void implicit_declaration(int x) {}
+
+int implicit_declaration_k_and_r(x) int x;
+{
+    return x;
+}