From 49cded32ab4d92c8504e71fa122f485401d6cc9b Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Tue, 28 Mar 2023 00:54:55 +0100 Subject: [PATCH 1/7] PRE31-C: Implement query Implements a heuristic query to find cases where arguments are provided to unsafe macros which have side-effects. --- .../SideEffectsInArgumentsToUnsafeMacros.md | 16 ++++ .../SideEffectsInArgumentsToUnsafeMacros.ql | 74 +++++++++++++++++++ ...eEffectsInArgumentsToUnsafeMacros.expected | 10 +++ ...SideEffectsInArgumentsToUnsafeMacros.qlref | 1 + c/cert/test/rules/PRE31-C/test.c | 29 ++++++++ cpp/common/src/codingstandards/cpp/Macro.qll | 3 +- .../cpp/exclusions/c/RuleMetadata.qll | 3 + .../cpp/exclusions/c/SideEffects4.qll | 26 +++++++ rule_packages/c/SideEffects4.json | 23 ++++++ rules.csv | 2 +- 10 files changed, 185 insertions(+), 2 deletions(-) create mode 100644 c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.md create mode 100644 c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql create mode 100644 c/cert/test/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.expected create mode 100644 c/cert/test/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.qlref create mode 100644 c/cert/test/rules/PRE31-C/test.c create mode 100644 cpp/common/src/codingstandards/cpp/exclusions/c/SideEffects4.qll create mode 100644 rule_packages/c/SideEffects4.json diff --git a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.md b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.md new file mode 100644 index 0000000000..e302785482 --- /dev/null +++ b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.md @@ -0,0 +1,16 @@ +# PRE31-C: Avoid side effects in arguments to unsafe macros + +This query implements the CERT-C rule PRE31-C: + +> Avoid side effects in arguments to unsafe macros +## CERT + +** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** + +## Implementation notes + +None + +## References + +* CERT-C: [PRE31-C: Avoid side effects in arguments to unsafe macros](https://wiki.sei.cmu.edu/confluence/display/c) diff --git a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql new file mode 100644 index 0000000000..ffd7334cb5 --- /dev/null +++ b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql @@ -0,0 +1,74 @@ +/** + * @id c/cert/side-effects-in-arguments-to-unsafe-macros + * @name PRE31-C: Avoid side effects in arguments to unsafe macros + * @description Macro arguments can be expanded multiple times which can cause side-effects to be + * evaluated multiple times. + * @kind problem + * @precision low + * @problem.severity error + * @tags external/cert/id/pre31-c + * correctness + * external/cert/obligation/rule + */ + +import cpp +import codingstandards.c.cert +import codingstandards.cpp.Macro +import codingstandards.cpp.SideEffect +import codingstandards.cpp.StructuralEquivalence +import codingstandards.cpp.sideeffect.DefaultEffects +import codingstandards.cpp.sideeffect.Customizations + +class FunctionCallEffect extends GlobalSideEffect::Range { + FunctionCallEffect() { + exists(Function f | + f = this.(FunctionCall).getTarget() and + // Not a side-effecting function + not f.(BuiltInFunction).getName() = "__builtin_expect" and + // Not side-effecting functions + not exists(string name | + name = + [ + "acos", "asin", "atan", "atan2", "ceil", "cos", "cosh", "exp", "fabs", "floor", "fmod", + "frexp", "ldexp", "log", "log10", "modf", "pow", "sin", "sinh", "sqrt", "tan", "tanh", + "cbrt", "erf", "erfc", "exp2", "expm1", "fdim", "fma", "fmax", "fmin", "hypot", "ilogb", + "lgamma", "llrint", "llround", "log1p", "log2", "logb", "lrint", "lround", "nan", + "nearbyint", "nextafter", "nexttoward", "remainder", "remquo", "rint", "round", + "scalbln", "scalbn", "tgamma", "trunc" + ] and + f.hasGlobalOrStdName([name, name + "f", name + "l"]) + ) + ) + } +} + +class CrementEffect extends LocalSideEffect::Range { + CrementEffect() { this instanceof CrementOperation } +} + +from + FunctionLikeMacro flm, MacroInvocation mi, Expr e, SideEffect sideEffect, int i, string arg, + string sideEffectDesc +where + not isExcluded(e, SideEffects4Package::sideEffectsInArgumentsToUnsafeMacrosQuery()) and + sideEffect = getASideEffect(e) and + flm.getAnInvocation() = mi and + not exists(mi.getParentInvocation()) and + mi.getAnExpandedElement() = e and + // Only consider arguments that are expanded multiple times, and do not consider "stringified" arguments + count(int index | index = flm.getAParameterUse(i) and not flm.getBody().charAt(index) = "#") > 1 and + arg = mi.getExpandedArgument(i) and + ( + sideEffect instanceof CrementEffect and + exists(arg.indexOf(sideEffect.(CrementOperation).getOperator())) and + sideEffectDesc = "the use of the " + sideEffect.(CrementOperation).getOperator() + " operator" + or + sideEffect instanceof FunctionCallEffect and + exists(arg.indexOf(sideEffect.(FunctionCall).getTarget().getName() + "(")) and + sideEffectDesc = + "a call to the function '" + sideEffect.(FunctionCall).getTarget().getName() + "'" + ) +select sideEffect, + "Argument " + mi.getUnexpandedArgument(i) + " to unsafe macro '" + flm.getName() + + "' is expanded to '" + arg + "' multiple times and includes " + sideEffectDesc + + " as a side-effect." diff --git a/c/cert/test/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.expected b/c/cert/test/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.expected new file mode 100644 index 0000000000..769d0c81c9 --- /dev/null +++ b/c/cert/test/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.expected @@ -0,0 +1,10 @@ +| test.c:9:10:9:12 | ... ++ | Argument i++ to unsafe macro 'unsafe' is expanded to 'i++' multiple times and includes the use of the ++ operator as a side-effect. | +| test.c:9:10:9:12 | ... ++ | Argument i++ to unsafe macro 'unsafe' is expanded to 'i++' multiple times and includes the use of the ++ operator as a side-effect. | +| test.c:11:10:11:12 | ... -- | Argument i-- to unsafe macro 'unsafe' is expanded to 'i--' multiple times and includes the use of the -- operator as a side-effect. | +| test.c:11:10:11:12 | ... -- | Argument i-- to unsafe macro 'unsafe' is expanded to 'i--' multiple times and includes the use of the -- operator as a side-effect. | +| test.c:26:10:26:15 | call to addOne | Argument addOne(10) to unsafe macro 'unsafe' is expanded to 'addOne(10)' multiple times and includes a call to the function 'addOne' as a side-effect. | +| test.c:26:10:26:15 | call to addOne | Argument addOne(10) to unsafe macro 'unsafe' is expanded to 'addOne(10)' multiple times and includes a call to the function 'addOne' as a side-effect. | +| test.c:27:10:27:17 | call to external | Argument external() to unsafe macro 'unsafe' is expanded to 'external()' multiple times and includes a call to the function 'external' as a side-effect. | +| test.c:27:10:27:17 | call to external | Argument external() to unsafe macro 'unsafe' is expanded to 'external()' multiple times and includes a call to the function 'external' as a side-effect. | +| test.c:28:10:28:15 | call to writeX | Argument writeX(10) to unsafe macro 'unsafe' is expanded to 'writeX(10)' multiple times and includes a call to the function 'writeX' as a side-effect. | +| test.c:28:10:28:15 | call to writeX | Argument writeX(10) to unsafe macro 'unsafe' is expanded to 'writeX(10)' multiple times and includes a call to the function 'writeX' as a side-effect. | diff --git a/c/cert/test/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.qlref b/c/cert/test/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.qlref new file mode 100644 index 0000000000..25a8d53fae --- /dev/null +++ b/c/cert/test/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.qlref @@ -0,0 +1 @@ +rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql \ No newline at end of file diff --git a/c/cert/test/rules/PRE31-C/test.c b/c/cert/test/rules/PRE31-C/test.c new file mode 100644 index 0000000000..87ca535f2b --- /dev/null +++ b/c/cert/test/rules/PRE31-C/test.c @@ -0,0 +1,29 @@ +#include + +#define safe(x) ((x) + 1) +#define unsafe(x) (x) * (x) + +void test_crement() { + int i = 0; + safe(i++); // COMPLIANT + unsafe(i++); // NON_COMPLIANT + safe(i--); // COMPLIANT + unsafe(i--); // NON_COMPLIANT +} + +int addOne(int x) { return x + 1; } +int writeX(int x) { + printf("%d", x); + return x; +} + +int external(); + +void test_call() { + safe(addOne(10)); // COMPLIANT + safe(external()); // COMPLIANT + safe(writeX(10)); // COMPLIANT + unsafe(addOne(10)); // COMPLIANT + unsafe(external()); // NON_COMPLIANT + unsafe(writeX(10)); // NON_COMPLIANT +} \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/Macro.qll b/cpp/common/src/codingstandards/cpp/Macro.qll index 65d0321271..5760d65bd3 100644 --- a/cpp/common/src/codingstandards/cpp/Macro.qll +++ b/cpp/common/src/codingstandards/cpp/Macro.qll @@ -15,7 +15,8 @@ class FunctionLikeMacro extends Macro { int getAParameterUse(int index) { exists(string parameter | parameter = getParameter(index) | - result = this.getBody().indexOf(parameter) + // Find identifier tokens in the program that match the parameter name + exists(this.getBody().regexpFind("\\#?\\b" + parameter + "\\b", _, result)) ) } } diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll index f4aed38bab..a89f30f01e 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/RuleMetadata.qll @@ -50,6 +50,7 @@ import Preprocessor5 import Preprocessor6 import SideEffects1 import SideEffects2 +import SideEffects4 import SignalHandlers import StandardLibraryFunctionTypes import Statements1 @@ -113,6 +114,7 @@ newtype TCQuery = TPreprocessor6PackageQuery(Preprocessor6Query q) or TSideEffects1PackageQuery(SideEffects1Query q) or TSideEffects2PackageQuery(SideEffects2Query q) or + TSideEffects4PackageQuery(SideEffects4Query q) or TSignalHandlersPackageQuery(SignalHandlersQuery q) or TStandardLibraryFunctionTypesPackageQuery(StandardLibraryFunctionTypesQuery q) or TStatements1PackageQuery(Statements1Query q) or @@ -176,6 +178,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat isPreprocessor6QueryMetadata(query, queryId, ruleId, category) or isSideEffects1QueryMetadata(query, queryId, ruleId, category) or isSideEffects2QueryMetadata(query, queryId, ruleId, category) or + isSideEffects4QueryMetadata(query, queryId, ruleId, category) or isSignalHandlersQueryMetadata(query, queryId, ruleId, category) or isStandardLibraryFunctionTypesQueryMetadata(query, queryId, ruleId, category) or isStatements1QueryMetadata(query, queryId, ruleId, category) or diff --git a/cpp/common/src/codingstandards/cpp/exclusions/c/SideEffects4.qll b/cpp/common/src/codingstandards/cpp/exclusions/c/SideEffects4.qll new file mode 100644 index 0000000000..d48b4a562d --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/c/SideEffects4.qll @@ -0,0 +1,26 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype SideEffects4Query = TSideEffectsInArgumentsToUnsafeMacrosQuery() + +predicate isSideEffects4QueryMetadata(Query query, string queryId, string ruleId, string category) { + query = + // `Query` instance for the `sideEffectsInArgumentsToUnsafeMacros` query + SideEffects4Package::sideEffectsInArgumentsToUnsafeMacrosQuery() and + queryId = + // `@id` for the `sideEffectsInArgumentsToUnsafeMacros` query + "c/cert/side-effects-in-arguments-to-unsafe-macros" and + ruleId = "PRE31-C" and + category = "rule" +} + +module SideEffects4Package { + Query sideEffectsInArgumentsToUnsafeMacrosQuery() { + //autogenerate `Query` type + result = + // `Query` type for `sideEffectsInArgumentsToUnsafeMacros` query + TQueryC(TSideEffects4PackageQuery(TSideEffectsInArgumentsToUnsafeMacrosQuery())) + } +} diff --git a/rule_packages/c/SideEffects4.json b/rule_packages/c/SideEffects4.json new file mode 100644 index 0000000000..1e1fa2f9a8 --- /dev/null +++ b/rule_packages/c/SideEffects4.json @@ -0,0 +1,23 @@ +{ + "CERT-C": { + "PRE31-C": { + "properties": { + "obligation": "rule" + }, + "queries": [ + { + "description": "Macro arguments can be expanded multiple times which can cause side-effects to be evaluated multiple times.", + "kind": "problem", + "name": "Avoid side effects in arguments to unsafe macros", + "precision": "low", + "severity": "error", + "short_name": "SideEffectsInArgumentsToUnsafeMacros", + "tags": [ + "correctness" + ] + } + ], + "title": "Avoid side effects in arguments to unsafe macros" + } + } +} \ No newline at end of file diff --git a/rules.csv b/rules.csv index 35b1c7f44c..86809d0a61 100644 --- a/rules.csv +++ b/rules.csv @@ -586,7 +586,7 @@ c,CERT-C,POS52-C,OutOfScope,Rule,,,Do not perform operations that can block whil c,CERT-C,POS53-C,OutOfScope,Rule,,,Do not use more than one mutex for concurrent waiting operations on a condition variable,,,, c,CERT-C,POS54-C,OutOfScope,Rule,,,Detect and handle POSIX library errors,,,, c,CERT-C,PRE30-C,No,Rule,,,Do not create a universal character name through concatenation,,,Medium, -c,CERT-C,PRE31-C,Yes,Rule,,,Avoid side effects in arguments to unsafe macros,RULE-13-2,SideEffects,Medium, +c,CERT-C,PRE31-C,Yes,Rule,,,Avoid side effects in arguments to unsafe macros,RULE-13-2,SideEffects4,Medium, c,CERT-C,PRE32-C,Yes,Rule,,,Do not use preprocessor directives in invocations of function-like macros,,Preprocessor5,Hard, c,CERT-C,SIG30-C,Yes,Rule,,,Call only asynchronous-safe functions within signal handlers,,SignalHandlers,Medium, c,CERT-C,SIG31-C,Yes,Rule,,,Do not access shared objects in signal handlers,,SignalHandlers,Medium, From d6a4c4e2b515f2c4a5fed721d1aa1f5702a64555 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 29 Mar 2023 11:40:32 +0100 Subject: [PATCH 2/7] PRE31-C: Create UnsafeMacro class. --- .../SideEffectsInArgumentsToUnsafeMacros.ql | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql index ffd7334cb5..555bf74a61 100644 --- a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql +++ b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql @@ -46,8 +46,27 @@ class CrementEffect extends LocalSideEffect::Range { CrementEffect() { this instanceof CrementOperation } } +/** + * A macro that is considered potentially "unsafe" because one or more arguments are expanded + * multiple times. + */ +class UnsafeMacro extends FunctionLikeMacro { + int unsafeArgumentIndex; + + UnsafeMacro() { + exists(this.getAParameterUse(unsafeArgumentIndex)) and + // Only consider arguments that are expanded multiple times, and do not consider "stringified" arguments + count(int indexInBody | + indexInBody = this.getAParameterUse(unsafeArgumentIndex) and + not this.getBody().charAt(indexInBody) = "#" + ) > 1 + } + + int getAnUnsafeArgumentIndex() { result = unsafeArgumentIndex } +} + from - FunctionLikeMacro flm, MacroInvocation mi, Expr e, SideEffect sideEffect, int i, string arg, + UnsafeMacro flm, MacroInvocation mi, Expr e, SideEffect sideEffect, int i, string arg, string sideEffectDesc where not isExcluded(e, SideEffects4Package::sideEffectsInArgumentsToUnsafeMacrosQuery()) and @@ -55,8 +74,7 @@ where flm.getAnInvocation() = mi and not exists(mi.getParentInvocation()) and mi.getAnExpandedElement() = e and - // Only consider arguments that are expanded multiple times, and do not consider "stringified" arguments - count(int index | index = flm.getAParameterUse(i) and not flm.getBody().charAt(index) = "#") > 1 and + i = flm.getAnUnsafeArgumentIndex() and arg = mi.getExpandedArgument(i) and ( sideEffect instanceof CrementEffect and From bef5a3621c7f3d4ddb1dbc56cc7a4e9045520412 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 29 Mar 2023 11:53:29 +0100 Subject: [PATCH 3/7] PRE31-C: Refactor to create UnsafeMacroInvocation Refactor the query to create a separate class for representing unsafe macro invocations. This will enable the query to be improved by determining whether we actually observe multiple side-effects in practice (to handle cases like `type(e) = e;`. --- .../SideEffectsInArgumentsToUnsafeMacros.ql | 49 +++++++++++++------ 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql index 555bf74a61..a4aaa8783f 100644 --- a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql +++ b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql @@ -65,28 +65,49 @@ class UnsafeMacro extends FunctionLikeMacro { int getAnUnsafeArgumentIndex() { result = unsafeArgumentIndex } } +/** + * An invocation of a potentially unsafe macro. + */ +class UnsafeMacroInvocation extends MacroInvocation { + UnsafeMacroInvocation() { + this.getMacro() instanceof UnsafeMacro and not exists(this.getParentInvocation()) + } + + /** + * Gets a side-effect for a potentially unsafe argument to the macro. + */ + SideEffect getSideEffectForUnsafeArg(int index) { + index = this.getMacro().(UnsafeMacro).getAnUnsafeArgumentIndex() and + exists(Expr e, string arg | + arg = this.getExpandedArgument(index) and + e = this.getAnExpandedElement() and + result = getASideEffect(e) and + ( + result instanceof CrementEffect and + exists(arg.indexOf(result.(CrementOperation).getOperator())) + or + result instanceof FunctionCallEffect and + exists(arg.indexOf(result.(FunctionCall).getTarget().getName() + "(")) + ) + ) + } +} + from - UnsafeMacro flm, MacroInvocation mi, Expr e, SideEffect sideEffect, int i, string arg, - string sideEffectDesc + UnsafeMacroInvocation unsafeMacroInvocation, SideEffect sideEffect, int i, string sideEffectDesc where - not isExcluded(e, SideEffects4Package::sideEffectsInArgumentsToUnsafeMacrosQuery()) and - sideEffect = getASideEffect(e) and - flm.getAnInvocation() = mi and - not exists(mi.getParentInvocation()) and - mi.getAnExpandedElement() = e and - i = flm.getAnUnsafeArgumentIndex() and - arg = mi.getExpandedArgument(i) and + not isExcluded(sideEffect, SideEffects4Package::sideEffectsInArgumentsToUnsafeMacrosQuery()) and + sideEffect = unsafeMacroInvocation.getSideEffectForUnsafeArg(i) and ( sideEffect instanceof CrementEffect and - exists(arg.indexOf(sideEffect.(CrementOperation).getOperator())) and sideEffectDesc = "the use of the " + sideEffect.(CrementOperation).getOperator() + " operator" or sideEffect instanceof FunctionCallEffect and - exists(arg.indexOf(sideEffect.(FunctionCall).getTarget().getName() + "(")) and sideEffectDesc = "a call to the function '" + sideEffect.(FunctionCall).getTarget().getName() + "'" ) select sideEffect, - "Argument " + mi.getUnexpandedArgument(i) + " to unsafe macro '" + flm.getName() + - "' is expanded to '" + arg + "' multiple times and includes " + sideEffectDesc + - " as a side-effect." + "Argument " + unsafeMacroInvocation.getUnexpandedArgument(i) + " to unsafe macro '" + + unsafeMacroInvocation.getMacroName() + "' is expanded to '" + + unsafeMacroInvocation.getExpandedArgument(i) + "' multiple times and includes " + sideEffectDesc + + " as a side-effect." From 272df6c05f66b4f0873a88c008dfa95ac68b5712 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 29 Mar 2023 12:03:17 +0100 Subject: [PATCH 4/7] PRE31-C: Ensure we see multiple side-effects A macro argument can be referred to multiple times in the body of a macro without it necessarily being evaluated multiple times - for example using an expression with sizeof, type etc. To handle these cases we ensure that we see multiple equivalent side-effects. --- .../PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql index a4aaa8783f..9b0d3155ff 100644 --- a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql +++ b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql @@ -18,6 +18,7 @@ import codingstandards.cpp.SideEffect import codingstandards.cpp.StructuralEquivalence import codingstandards.cpp.sideeffect.DefaultEffects import codingstandards.cpp.sideeffect.Customizations +import semmle.code.cpp.valuenumbering.HashCons class FunctionCallEffect extends GlobalSideEffect::Range { FunctionCallEffect() { @@ -100,9 +101,20 @@ where sideEffect = unsafeMacroInvocation.getSideEffectForUnsafeArg(i) and ( sideEffect instanceof CrementEffect and + // Do we observe the same side-effect multiple times? + count(SideEffect equivalentSideEffect | + equivalentSideEffect = unsafeMacroInvocation.getSideEffectForUnsafeArg(i) and + hashCons(equivalentSideEffect.(CrementOperation).getOperand()) = + hashCons(sideEffect.(CrementOperation).getOperand()) + ) > 1 and sideEffectDesc = "the use of the " + sideEffect.(CrementOperation).getOperator() + " operator" or sideEffect instanceof FunctionCallEffect and + // Do we observe the same side-effect multiple times? + count(SideEffect equivalentSideEffect | + equivalentSideEffect = unsafeMacroInvocation.getSideEffectForUnsafeArg(i) and + equivalentSideEffect.(FunctionCall).getTarget() = sideEffect.(FunctionCall).getTarget() + ) > 1 and sideEffectDesc = "a call to the function '" + sideEffect.(FunctionCall).getTarget().getName() + "'" ) From fc519484e305cb7ba20b7f13a274b093a5f3cb15 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 29 Mar 2023 12:05:04 +0100 Subject: [PATCH 5/7] PRE31-C: Improve docs --- .../SideEffectsInArgumentsToUnsafeMacros.ql | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql index 9b0d3155ff..ae2376f4e0 100644 --- a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql +++ b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql @@ -23,10 +23,11 @@ import semmle.code.cpp.valuenumbering.HashCons class FunctionCallEffect extends GlobalSideEffect::Range { FunctionCallEffect() { exists(Function f | + // Capture function calls as side-effects f = this.(FunctionCall).getTarget() and - // Not a side-effecting function + // Excluding __builtin_expect, which is not a side-effecting function not f.(BuiltInFunction).getName() = "__builtin_expect" and - // Not side-effecting functions + // Excluding common math functions not exists(string name | name = [ @@ -80,13 +81,20 @@ class UnsafeMacroInvocation extends MacroInvocation { SideEffect getSideEffectForUnsafeArg(int index) { index = this.getMacro().(UnsafeMacro).getAnUnsafeArgumentIndex() and exists(Expr e, string arg | - arg = this.getExpandedArgument(index) and e = this.getAnExpandedElement() and result = getASideEffect(e) and + // Unfortunately, there's no semantic way to check whether a particular expression or + // side-effect generated by a macro came from a particular macro argument. The only + // information we get is the string of the expanded argument. We therefore do some basic + // string matching to check whether it looks like this side-effect comes from the given + // argument + arg = this.getExpandedArgument(index) and ( + // If this is a crement effect, then check that the text of the macro argument includes -- or ++ result instanceof CrementEffect and exists(arg.indexOf(result.(CrementOperation).getOperator())) or + // If this is a functional call effect, then check that the text of the macro argument includes a call to that function result instanceof FunctionCallEffect and exists(arg.indexOf(result.(FunctionCall).getTarget().getName() + "(")) ) From 264f3cd405fa8962e3c6270a333cdbfe33534d79 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 29 Mar 2023 12:06:08 +0100 Subject: [PATCH 6/7] PRE31-C: CERT help text --- .../SideEffectsInArgumentsToUnsafeMacros.md | 197 +++++++++++++++++- 1 file changed, 195 insertions(+), 2 deletions(-) diff --git a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.md b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.md index e302785482..55bb78c0ae 100644 --- a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.md +++ b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.md @@ -3,9 +3,202 @@ This query implements the CERT-C rule PRE31-C: > Avoid side effects in arguments to unsafe macros -## CERT -** REPLACE THIS BY RUNNING THE SCRIPT `scripts/help/cert-help-extraction.py` ** + +## Description + +An [unsafe function-like macro](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-unsafefunction-likemacro) is one whose expansion results in evaluating one of its parameters more than once or not at all. Never invoke an unsafe macro with arguments containing an assignment, increment, decrement, volatile access, input/output, or other expressions with side effects (including function calls, which may cause side effects). + +The documentation for unsafe macros should warn against invoking them with arguments with side effects, but the responsibility is on the programmer using the macro. Because of the risks associated with their use, it is recommended that the creation of unsafe function-like macros be avoided. (See [PRE00-C. Prefer inline or static functions to function-like macros](https://wiki.sei.cmu.edu/confluence/display/c/PRE00-C.+Prefer+inline+or+static+functions+to+function-like+macros).) + +This rule is similar to [EXP44-C. Do not rely on side effects in operands to sizeof, _Alignof, or _Generic](https://wiki.sei.cmu.edu/confluence/display/c/EXP44-C.+Do+not+rely+on+side+effects+in+operands+to+sizeof%2C+_Alignof%2C+or+_Generic). + +## Noncompliant Code Example + +One problem with unsafe macros is [side effects](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-sideeffect) on macro arguments, as shown by this noncompliant code example: + +```cpp +#define ABS(x) (((x) < 0) ? -(x) : (x)) + +void func(int n) { + /* Validate that n is within the desired range */ + int m = ABS(++n); + + /* ... */ +} +``` +The invocation of the `ABS()` macro in this example expands to + +```cpp +m = (((++n) < 0) ? -(++n) : (++n)); + +``` +The resulting code is well defined but causes `n` to be incremented twice rather than once. + +## Compliant Solution + +In this compliant solution, the increment operation `++n` is performed before the call to the unsafe macro. + +```cpp +#define ABS(x) (((x) < 0) ? -(x) : (x)) /* UNSAFE */ + +void func(int n) { + /* Validate that n is within the desired range */ + ++n; + int m = ABS(n); + + /* ... */ +} +``` +Note the comment warning programmers that the macro is unsafe. The macro can also be renamed `ABS_UNSAFE()` to make it clear that the macro is unsafe. This compliant solution, like all the compliant solutions for this rule, has undefined behavior if the argument to `ABS()` is equal to the minimum (most negative) value for the signed integer type. (See [INT32-C. Ensure that operations on signed integers do not result in overflow](https://wiki.sei.cmu.edu/confluence/display/c/INT32-C.+Ensure+that+operations+on+signed+integers+do+not+result+in+overflow) for more information.) + +## Compliant Solution + +This compliant solution follows the guidance of [PRE00-C. Prefer inline or static functions to function-like macros](https://wiki.sei.cmu.edu/confluence/display/c/PRE00-C.+Prefer+inline+or+static+functions+to+function-like+macros) by defining an inline function `iabs()` to replace the `ABS()` macro. Unlike the `ABS()` macro, which operates on operands of any type, the `iabs()` function will truncate arguments of types wider than `int` whose value is not in range of the latter type. + +```cpp +#include +#include + +static inline int iabs(int x) { + return (((x) < 0) ? -(x) : (x)); +} + +void func(int n) { + /* Validate that n is within the desired range */ + +int m = iabs(++n); + + /* ... */ +} +``` + +## Compliant Solution + +A more flexible compliant solution is to declare the `ABS()` macro using a `_Generic` selection. To support all arithmetic data types, this solution also makes use of inline functions to compute integer absolute values. (See [PRE00-C. Prefer inline or static functions to function-like macros](https://wiki.sei.cmu.edu/confluence/display/c/PRE00-C.+Prefer+inline+or+static+functions+to+function-like+macros) and [PRE12-C. Do not define unsafe macros](https://wiki.sei.cmu.edu/confluence/display/c/PRE12-C.+Do+not+define+unsafe+macros).) + +According to the C Standard, 6.5.1.1, paragraph 3 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO%2FIEC9899-2011)\]: + +> The controlling expression of a generic selection is not evaluated. If a generic selection has a generic association with a type name that is compatible with the type of the controlling expression, then the result expression of the generic selection is the expression in that generic association. Otherwise, the result expression of the generic selection is the expression in the `default` generic association. None of the expressions from any other generic association of the generic selection is evaluated. + + +Because the expression is not evaluated as part of the generic selection, the use of a macro in this solution is guaranteed to evaluate the macro parameter `v` only once. + +```cpp +#include +#include + +static inline long long llabs(long long v) { + return v < 0 ? -v : v; +} +static inline long labs(long v) { + return v < 0 ? -v : v; +} +static inline int iabs(int v) { + return v < 0 ? -v : v; +} +static inline int sabs(short v) { + return v < 0 ? -v : v; +} +static inline int scabs(signed char v) { + return v < 0 ? -v : v; +} + +#define ABS(v) _Generic(v, signed char : scabs, \ + short : sabs, \ + int : iabs, \ + long : labs, \ + long long : llabs, \ + float : fabsf, \ + double : fabs, \ + long double : fabsl, \ + double complex : cabs, \ + float complex : cabsf, \ + long double complex : cabsl)(v) + +void func(int n) { + /* Validate that n is within the desired range */ + int m = ABS(++n); + /* ... */ +} +``` +Generic selections were introduced in C11 and are not available in C99 and earlier editions of the C Standard. + +## Compliant Solution (GCC) + +GCC's [__typeof](http://gcc.gnu.org/onlinedocs/gcc/Typeof.html) extension makes it possible to declare and assign the value of the macro operand to a temporary of the same type and perform the computation on the temporary, consequently guaranteeing that the operand will be evaluated exactly once. Another GCC extension, known as *statement expression[](http://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html)*, makes it possible for the block statement to appear where an expression is expected: + +```cpp +#define ABS(x) __extension__ ({ __typeof (x) tmp = x; \ + tmp < 0 ? -tmp : tmp; }) +``` +Note that relying on such extensions makes code nonportable and violates [MSC14-C. Do not introduce unnecessary platform dependencies](https://wiki.sei.cmu.edu/confluence/display/c/MSC14-C.+Do+not+introduce+unnecessary+platform+dependencies). + +## Noncompliant Code Example (assert()) + +The `assert()` macro is a convenient mechanism for incorporating diagnostic tests in code. (See [MSC11-C. Incorporate diagnostic tests using assertions](https://wiki.sei.cmu.edu/confluence/display/c/MSC11-C.+Incorporate+diagnostic+tests+using+assertions).) Expressions used as arguments to the standard `assert()` macro should not have side effects. The behavior of the `assert()` macro depends on the definition of the object-like macro `NDEBUG`. If the macro `NDEBUG` is undefined, the `assert()` macro is defined to evaluate its expression argument and, if the result of the expression compares equal to 0, call the `abort()` function. If `NDEBUG` is defined, `assert` is defined to expand to `((void)0)`. Consequently, the expression in the assertion is not evaluated, and no side effects it may have had otherwise take place in non-debugging executions of the code. + +This noncompliant code example includes an `assert()` macro containing an expression (`index++`) that has a side effect: + +```cpp +#include +#include + +void process(size_t index) { + assert(index++ > 0); /* Side effect */ + /* ... */ +} + +``` + +## Compliant Solution (assert()) + +This compliant solution avoids the possibility of side effects in assertions by moving the expression containing the side effect outside of the `assert()` macro. + +```cpp +#include +#include + +void process(size_t index) { + assert(index > 0); /* No side effect */ + ++index; + /* ... */ +} +``` + +## Exceptions + +**PRE31-C-EX1:** An exception can be made for invoking an [unsafe macro](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-unsafefunction-likemacro) with a function call argument provided that the function has no [side effects](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-sideeffect). However, it is easy to forget about obscure side effects that a function might have, especially library functions for which source code is not available; even changing `errno` is a side effect. Unless the function is user-written and does nothing but perform a computation and return its result without calling any other functions, it is likely that many developers will forget about some side effect. Consequently, this exception must be used with great care. + +## Risk Assessment + +Invoking an unsafe macro with an argument that has side effects may cause those side effects to occur more than once. This practice can lead to [unexpected program behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-unexpectedbehavior). + +
Rule Severity Likelihood Remediation Cost Priority Level
PRE31-C Low Unlikely Low P3 L3
+ + +## Automated Detection + +
Tool Version Checker Description
Astrée 22.04 expanded-side-effect-multiplied expanded-side-effect-not-evaluated side-effect-not-expanded Partially checked
Axivion Bauhaus Suite 7.2.0 CertC-PRE31 Fully implemented
CodeSonar 7.2p0 LANG.PREPROC.FUNCMACRO LANG.STRUCT.SE.DEC LANG.STRUCT.SE.INC Function-Like Macro Side Effects in Expression with Decrement Side Effects in Expression with Increment
Coverity 2017.07 ASSERT_SIDE_EFFECTS Partially implemented Can detect the specific instance where assertion contains an operation/function call that may have a side effect
ECLAIR 1.2 CC2.EXP31CC2.PRE31 Fully implemented
Helix QAC 2022.4 C3462, C3463, C3464, C3465, C3466, C3467 C++3225, C++3226, C++3227, C++3228, C++3229
Klocwork 2022.4 PORTING.VAR.EFFECTS
LDRA tool suite 9.7.1 9 S, 562 S, 572 S, 35 D, 1 Q Fully implemented
Parasoft C/C++test 2022.2 CERT_C-PRE31-b CERT_C-PRE31-c CERT_C-PRE31-d Assertions should not contain assignments, increment, or decrement operators Assertions should not contain function calls nor function-like macro calls Avoid side effects in arguments to unsafe macros
PC-lint Plus 1.4 666, 2666 Fully supported
Polyspace Bug Finder R2023a CERT C: Rule PRE31-C Checks for side effect in arguments to unsafe macro (rule partially covered)
PRQA QA-C 9.7 3462, 3463, 3464, 3465, 3466, 3467 Fully implemented
PRQA QA-C++ 4.4 3225, 3226, 3227, 3228, 3229
RuleChecker 22.04 expanded-side-effect-multiplied expanded-side-effect-not-evaluated side-effect-not-expanded Partially checked
+ + +## Related Vulnerabilities + +Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+PRE31-C). + +## Related Guidelines + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions) + +
Taxonomy Taxonomy item Relationship
+ + +## Bibliography + +
\[ Dewhurst 2002 \] Gotcha \#28, "Side Effects in Assertions"
\[ ISO/IEC 9899:2011 \] Subclause 6.5.1.1, "Generic Selection"
\[ Plum 1985 \] Rule 1-11
+ ## Implementation notes From edca7d12bc92d264a7af4ab17d904d8a9a035793 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Wed, 29 Mar 2023 22:45:06 +0100 Subject: [PATCH 7/7] PRE31-C: Update metadata and docs * Add an implementation scope * Add qldocs * Update description. --- .../rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.md | 2 +- .../rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql | 8 +++++++- rule_packages/c/SideEffects4.json | 7 +++++-- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.md b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.md index 55bb78c0ae..9a83bee144 100644 --- a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.md +++ b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.md @@ -202,7 +202,7 @@ Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+D ## Implementation notes -None +This implementation only considers ++ and function call side effects. Due to the textual nature of macro expansion it is not always possible to determine accurately whether a side-effect was produced by a particular argument, and this may cause both false positives and false negatives. The query does not consider the case where a macro argument including a side-effect is never evaluated. ## References diff --git a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql index ae2376f4e0..4ae6619227 100644 --- a/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql +++ b/c/cert/src/rules/PRE31-C/SideEffectsInArgumentsToUnsafeMacros.ql @@ -2,7 +2,7 @@ * @id c/cert/side-effects-in-arguments-to-unsafe-macros * @name PRE31-C: Avoid side effects in arguments to unsafe macros * @description Macro arguments can be expanded multiple times which can cause side-effects to be - * evaluated multiple times. + * evaluated multiple times leading to unexpected program behavior. * @kind problem * @precision low * @problem.severity error @@ -20,6 +20,9 @@ import codingstandards.cpp.sideeffect.DefaultEffects import codingstandards.cpp.sideeffect.Customizations import semmle.code.cpp.valuenumbering.HashCons +/** + * Add side-effecting functions to the default set of side-effects. + */ class FunctionCallEffect extends GlobalSideEffect::Range { FunctionCallEffect() { exists(Function f | @@ -44,6 +47,9 @@ class FunctionCallEffect extends GlobalSideEffect::Range { } } +/** + * Add crement operations to the default set of side-effects. + */ class CrementEffect extends LocalSideEffect::Range { CrementEffect() { this instanceof CrementOperation } } diff --git a/rule_packages/c/SideEffects4.json b/rule_packages/c/SideEffects4.json index 1e1fa2f9a8..77121019de 100644 --- a/rule_packages/c/SideEffects4.json +++ b/rule_packages/c/SideEffects4.json @@ -6,7 +6,7 @@ }, "queries": [ { - "description": "Macro arguments can be expanded multiple times which can cause side-effects to be evaluated multiple times.", + "description": "Macro arguments can be expanded multiple times which can cause side-effects to be evaluated multiple times leading to unexpected program behavior.", "kind": "problem", "name": "Avoid side effects in arguments to unsafe macros", "precision": "low", @@ -14,7 +14,10 @@ "short_name": "SideEffectsInArgumentsToUnsafeMacros", "tags": [ "correctness" - ] + ], + "implementation_scope": { + "description": "This implementation only considers ++ and function call side effects. Due to the textual nature of macro expansion it is not always possible to determine accurately whether a side-effect was produced by a particular argument, and this may cause both false positives and false negatives. The query does not consider the case where a macro argument including a side-effect is never evaluated." + } } ], "title": "Avoid side effects in arguments to unsafe macros"