[CPP-340] Refinements to FutileParams.ql etc. #1136
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| void three_arguments(int x, int y, int z); | ||
|
|
||
| void calls() { | ||
| int three = 3; | ||
| three_arguments(1, 2, three); // GOOD | ||
| three_arguments(1, 2, &three); // BAD | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
|
|
||
|
|
||
| <overview> | ||
| <p>A function is called with at least one argument whose type is incompatible with the type of | ||
| the corresponding parameter of the function being called. This may cause the called function | ||
| to behave unpredictably.</p> | ||
|
|
||
| <p>This may indicate that an incorrect function is being called, or that the | ||
| signature (parameter list and parameter types) of the called function | ||
| is not known to the author.</p> | ||
|
|
||
| </overview> | ||
| <recommendation> | ||
| <p>Call the function with the proper argument types. In some cases, it may | ||
| suffice to provide an explicit cast of an argument to the desired (parameter) type.</p> | ||
|
|
||
| </recommendation> | ||
| <example><sample src="MistypedFunctionArguments.c" /> | ||
|
|
||
| </example> | ||
|
|
||
| <references> | ||
| <li>SEI CERT C Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/c/DCL20-C.+Explicitly+specify+void+when+a+function+accepts+no+arguments"> DCL20-C. Explicitly specify void when a function accepts no arguments </a></li> | ||
| </references> | ||
| </qhelp> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| /** | ||
| * @name Call to a function with one or more incompatible arguments | ||
| * @description When the type of a function argument is not compatible | ||
| * with the type of the corresponding parameter, it may lead to | ||
| * unpredictable behavior. | ||
| * @kind problem | ||
| * @problem.severity warning | ||
| * @precision medium | ||
| * @id cpp/mistyped-function-arguments | ||
| * @tags correctness | ||
| * maintainability | ||
| */ | ||
|
|
||
| import cpp | ||
|
|
||
| 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] | ||
|
Contributor
There was a problem hiding this comment. This predicate has changed enough that its comment is no longer relevant. Since it's only called from one place, I suggest inlining it by hand, i.e., removing it.
Contributor
Author
There was a problem hiding this comment. I agree that the comment is not terribly useful. However, I prefer to separate things into predicates as they improve code readability. Is there a run-time performance hit for doing so?
Contributor
There was a problem hiding this comment. There's almost no run-time difference between using In any case, the comment needs to go. It doesn't describe this predicate. |
||
| predicate argMayBeUsed(Expr arg, Parameter parm) { | ||
| argTypeMayBeUsed(arg.getFullyConverted().getType().getUnspecifiedType(), | ||
| parm.getType().getUnspecifiedType()) | ||
| } | ||
|
|
||
| // True if function was ()-declared, but not (void)-declared or K&R-defined | ||
| predicate hasZeroParamDecl(Function f) { | ||
|
Contributor
Author
There was a problem hiding this comment. Because I thought it would improve performance. Is this not the case?
Contributor
There was a problem hiding this comment. I think it'll make performance worse in this case. Generally in QL it's best to leave inlining decisions to the compiler. |
||
| 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(Function f) { | ||
| exists(File file | file.compiledAsC() | | ||
| file = f.getFile() or file.getAnIncludedFile+() = f.getFile() | ||
| ) | ||
| } | ||
|
|
||
| from FunctionCall fc, Function f, Parameter p | ||
| where | ||
| f = fc.getTarget() and | ||
| p = f.getAParameter() and | ||
| hasZeroParamDecl(f) and | ||
| isCompiledAsC(f) 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().getType().getUnspecifiedType() as atype, atype.toString(), p, | ||
| p.getTypedName() | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| void one_argument(); | ||
|
|
||
| void calls() { | ||
| one_argument(1); // GOOD: `one_argument` will accept and use the argument | ||
|
|
||
| one_argument(); // BAD: `one_argument` will receive an undefined value | ||
| } | ||
|
|
||
| void one_argument(int x); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
|
|
||
|
|
||
| <overview> | ||
| <p>A function is called with fewer arguments than there are parameters of the function.</p> | ||
|
|
||
| <p>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.</p> | ||
|
|
||
| <p>In C, function calls generally need to provide the same number of arguments as there are | ||
| arguments to the function. (Variadic functions can accept additional arguments.) Providing | ||
| fewer arguments than there are parameters is extremely dangerous, as the called function | ||
| will nevertheless try to obtain the missing arguments' values, either from the stack | ||
| or from machine registers. As a result, the function may behave unpredictably.</p> | ||
|
|
||
| <p>If the called function <i>modifies</i> a parameter corresponding to a missing argument, it | ||
| may alter the state of the program upon its return. An attacker could use this to, | ||
| for example, alter the control flow of the program to access forbidden resources.</p> | ||
|
|
||
| </overview> | ||
| <recommendation> | ||
| <p>Call the function with the correct number of arguments.</p> | ||
|
|
||
| </recommendation> | ||
| <example><sample src="TooFewArguments.c" /> | ||
|
|
||
| </example> | ||
|
|
||
| <references> | ||
| <li>SEI CERT C Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/c/DCL20-C.+Explicitly+specify+void+when+a+function+accepts+no+arguments"> DCL20-C. Explicitly specify void when a function accepts no arguments </a></li> | ||
| </references> | ||
| </qhelp> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| /** | ||
| * @name Call to function with fewer arguments than declared parameters | ||
| * @description A function call is passing fewer arguments than the number of | ||
| * declared parameters of the function. This may indicate | ||
| * that the code does not follow the author's intent. It is also | ||
| * a vulnerability, since the function is likely to operate on | ||
| * undefined data. | ||
| * @kind problem | ||
| * @problem.severity error | ||
| * @precision very-high | ||
| * @id cpp/too-few-arguments | ||
| * @tags correctness | ||
| * maintainability | ||
| * security | ||
| */ | ||
|
|
||
| import cpp | ||
|
|
||
| // 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(Function f) { | ||
| exists(File file | file.compiledAsC() | | ||
| file = f.getFile() or file.getAnIncludedFile+() = f.getFile() | ||
| ) | ||
| } | ||
|
|
||
| from FunctionCall fc, Function f | ||
| where | ||
| f = fc.getTarget() and | ||
| not f.isVarargs() and | ||
| not f instanceof BuiltInFunction and | ||
| hasZeroParamDecl(f) and | ||
| isCompiledAsC(f) 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() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| void one_argument(); | ||
|
|
||
| void calls() { | ||
|
|
||
| one_argument(1); // GOOD: `one_argument` will accept and use the argument | ||
|
|
||
| one_argument(1, 2); // BAD: `one_argument` will use the first argument but ignore the second | ||
| } | ||
|
|
||
| void one_argument(int x); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
|
|
||
|
|
||
| <overview> | ||
| <p>A function is called with more arguments than there are parameters of the function.</p> | ||
|
|
||
| <p>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.</p> | ||
|
|
||
| <p>In C, function calls generally need to provide the same number of arguments as there are | ||
| arguments to the function. (Variadic functions can accept additional arguments.) Providing | ||
| more arguments than there are parameters incurs an unneeded computational overhead, both | ||
| in terms of time and of additional stack space.</p> | ||
|
|
||
| </overview> | ||
| <recommendation> | ||
| <p>Call the function with the correct number of arguments.</p> | ||
|
|
||
| </recommendation> | ||
| <example><sample src="TooManyArguments.c" /> | ||
|
|
||
| </example> | ||
|
|
||
| <references> | ||
| <li>SEI CERT C Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/c/DCL20-C.+Explicitly+specify+void+when+a+function+accepts+no+arguments"> DCL20-C. Explicitly specify void when a function accepts no arguments </a></li> | ||
| </references> | ||
| </qhelp> |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's possible for a fully-converted argument to be an
ArrayType. It'll undergo an array-to-pointer conversion at some point. That means you can eliminate the last two of these cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I was thinking of C99/C11 fixed-length array parameters (e.g.,
int arr[ static 10]), but upon further research discovered that thestatic 10is merely a codegen hint for the compiler and that a raw pointer is still passed in.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it some more, and now I think you do need the
arg.(ArrayType)cases in order to handle the rarely-used "pointer to array" types (declared likeint (*parameterName)[2]).But hang on, where is the recursion that would enable this? If you allow passing a
int *to aunsigned int *argument, shouldn't you also allow passing anint **to aunsigned int **argument? But recursive predicates can't be inline, so you can't support this without some serious restructuring for the sake of performance.To start with, please make sure there is test coverage for "pointer to array" and "pointer to pointer" to compatible-but-not-identical base types.