diff --git a/.vscode/tasks.json b/.vscode/tasks.json index 82cad087ae..32839c8ce8 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -259,6 +259,7 @@ "Language1", "Language2", "Language3", + "Lifetime", "Linkage1", "Linkage2", "Literals", diff --git a/change_notes/2026-02-03-uninitialized-mem-improve.md b/change_notes/2026-02-03-uninitialized-mem-improve.md new file mode 100644 index 0000000000..2fb2fd6539 --- /dev/null +++ b/change_notes/2026-02-03-uninitialized-mem-improve.md @@ -0,0 +1,2 @@ +- `A8-5-0`, `EXP53-CPP`, `EXP33-C`, `RULE-9-1` - `MemoryNotInitializedBeforeItIsRead.ql`, `DoNotReadUninitializedMemory.ql`, `DoNotReadUninitializedMemory.ql`, `ObjectWithAutoStorageDurationReadBeforeInit.ql`: + - The queries listed now find uses of the operator 'new' where there is no value initialization provided. \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/exclusions/cpp/Lifetime.qll b/cpp/common/src/codingstandards/cpp/exclusions/cpp/Lifetime.qll new file mode 100644 index 0000000000..bb06eaaf8f --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/exclusions/cpp/Lifetime.qll @@ -0,0 +1,44 @@ +//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/ +import cpp +import RuleMetadata +import codingstandards.cpp.exclusions.RuleMetadata + +newtype LifetimeQuery = + TValueOfAnObjectMustNotBeReadBeforeItHasBeenSetQuery() or + TAutomaticStorageAssignedToObjectGreaterLifetimeQuery() + +predicate isLifetimeQueryMetadata(Query query, string queryId, string ruleId, string category) { + query = + // `Query` instance for the `valueOfAnObjectMustNotBeReadBeforeItHasBeenSet` query + LifetimePackage::valueOfAnObjectMustNotBeReadBeforeItHasBeenSetQuery() and + queryId = + // `@id` for the `valueOfAnObjectMustNotBeReadBeforeItHasBeenSet` query + "cpp/misra/value-of-an-object-must-not-be-read-before-it-has-been-set" and + ruleId = "RULE-11-6-2" and + category = "mandatory" + or + query = + // `Query` instance for the `automaticStorageAssignedToObjectGreaterLifetime` query + LifetimePackage::automaticStorageAssignedToObjectGreaterLifetimeQuery() and + queryId = + // `@id` for the `automaticStorageAssignedToObjectGreaterLifetime` query + "cpp/misra/automatic-storage-assigned-to-object-greater-lifetime" and + ruleId = "RULE-6-8-3" and + category = "required" +} + +module LifetimePackage { + Query valueOfAnObjectMustNotBeReadBeforeItHasBeenSetQuery() { + //autogenerate `Query` type + result = + // `Query` type for `valueOfAnObjectMustNotBeReadBeforeItHasBeenSet` query + TQueryCPP(TLifetimePackageQuery(TValueOfAnObjectMustNotBeReadBeforeItHasBeenSetQuery())) + } + + Query automaticStorageAssignedToObjectGreaterLifetimeQuery() { + //autogenerate `Query` type + result = + // `Query` type for `automaticStorageAssignedToObjectGreaterLifetime` query + TQueryCPP(TLifetimePackageQuery(TAutomaticStorageAssignedToObjectGreaterLifetimeQuery())) + } +} diff --git a/cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll index 88537493d8..b9de050e38 100644 --- a/cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll +++ b/cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll @@ -45,6 +45,7 @@ import IntegerConversion import Invariants import Iterators import Lambdas +import Lifetime import Linkage1 import Linkage2 import Literals @@ -133,6 +134,7 @@ newtype TCPPQuery = TInvariantsPackageQuery(InvariantsQuery q) or TIteratorsPackageQuery(IteratorsQuery q) or TLambdasPackageQuery(LambdasQuery q) or + TLifetimePackageQuery(LifetimeQuery q) or TLinkage1PackageQuery(Linkage1Query q) or TLinkage2PackageQuery(Linkage2Query q) or TLiteralsPackageQuery(LiteralsQuery q) or @@ -221,6 +223,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat isInvariantsQueryMetadata(query, queryId, ruleId, category) or isIteratorsQueryMetadata(query, queryId, ruleId, category) or isLambdasQueryMetadata(query, queryId, ruleId, category) or + isLifetimeQueryMetadata(query, queryId, ruleId, category) or isLinkage1QueryMetadata(query, queryId, ruleId, category) or isLinkage2QueryMetadata(query, queryId, ruleId, category) or isLiteralsQueryMetadata(query, queryId, ruleId, category) or diff --git a/cpp/common/src/codingstandards/cpp/lifetimes/CppObjects.qll b/cpp/common/src/codingstandards/cpp/lifetimes/CppObjects.qll index 4ef3122805..f1d26047d2 100644 --- a/cpp/common/src/codingstandards/cpp/lifetimes/CppObjects.qll +++ b/cpp/common/src/codingstandards/cpp/lifetimes/CppObjects.qll @@ -262,6 +262,8 @@ class AggregateLiteralObjectIdentity extends AggregateLiteral, ObjectIdentityBas class AllocatedObjectIdentity extends AllocationExpr, ObjectIdentityBase { AllocatedObjectIdentity() { this.(FunctionCall).getTarget().(AllocationFunction).requiresDealloc() + or + this = any(NewOrNewArrayExpr new | not exists(new.getPlacementPointer())) } override StorageDuration getStorageDuration() { result.isAllocated() } diff --git a/cpp/common/src/codingstandards/cpp/rules/readofuninitializedmemory/InitializationFunctions.qll b/cpp/common/src/codingstandards/cpp/rules/readofuninitializedmemory/InitializationFunctions.qll new file mode 100644 index 0000000000..ca68fd526d --- /dev/null +++ b/cpp/common/src/codingstandards/cpp/rules/readofuninitializedmemory/InitializationFunctions.qll @@ -0,0 +1,687 @@ +/** + * Provides classes and predicates for identifying functions that initialize their arguments. + * This is a copy of the out of the box library that is in a query specific library + * https://github.com/github/codeql/blob/main/cpp/ql/src/Security/CWE/CWE-457/InitializationFunctions.qll + * at cc7a9ef. + */ + +import cpp +import external.ExternalArtifact +private import semmle.code.cpp.dispatch.VirtualDispatchPrototype +import semmle.code.cpp.NestedFields +import semmle.code.cpp.controlflow.Guards + +/** A context under which a function may be called. */ +private newtype TContext = + /** No specific call context. */ + NoContext() or + /** + * The call context is that the given other parameter is null. + * + * This context is created for all parameters that are null checked in the body of the function. + */ + ParamNull(Parameter p) { p = any(ParameterNullCheck pnc).getParameter() } or + /** + * The call context is that the given other parameter is not null. + * + * This context is created for all parameters that are null checked in the body of the function. + */ + ParamNotNull(Parameter p) { p = any(ParameterNullCheck pnc).getParameter() } + +/** + * A context under which a function may be called. + * + * Some functions may conditionally initialize a parameter depending on the value of another + * parameter. Consider: + * ``` + * int MyInitFunction(int* paramToBeInitialized, int* paramToCheck) { + * if (!paramToCheck) { + * // fail! + * return -1; + * } + * paramToBeInitialized = 0; + * } + * ``` + * In this case, whether `paramToBeInitialized` is initialized when this function call completes + * depends on whether `paramToCheck` is or is not null. A call-context insensitive analysis will + * determine that any call to this function may leave the parameter uninitialized, even if the + * argument to paramToCheck is guaranteed to be non-null (`&foo`, for example). + * + * This class models call contexts that can be considered when calculating whether a given parameter + * initializes or not. The supported contexts are: + * - `ParamNull(otherParam)` - the given `otherParam` is considered to be null. Applies when + * exactly one parameter other than this one is null checked. + * - `ParamNotNull(otherParam)` - the given `otherParam` is considered to be not null. Applies when + * exactly one parameter other than this one is null checked. + * - `NoContext()` - applies in all other circumstances. + */ +class Context extends TContext { + string toString() { + this = NoContext() and result = "NoContext" + or + this = ParamNull(any(Parameter p | result = "ParamNull(" + p.getName() + ")")) + or + this = ParamNotNull(any(Parameter p | result = "ParamNotNull(" + p.getName() + ")")) + } +} + +/** + * A check against a parameter. + */ +abstract class ParameterCheck extends Expr { + /** + * Gets a successor of this check that should be ignored for the given context. + */ + abstract ControlFlowNode getIgnoredSuccessorForContext(Context c); +} + +/** A null-check expression on a parameter. */ +class ParameterNullCheck extends ParameterCheck { + Parameter p; + ControlFlowNode nullSuccessor; + ControlFlowNode notNullSuccessor; + + ParameterNullCheck() { + this.isCondition() and + p.getFunction() instanceof InitializationFunction and + p.getType().getUnspecifiedType() instanceof PointerType and + exists(VariableAccess va | va = p.getAnAccess() | + nullSuccessor = this.getATrueSuccessor() and + notNullSuccessor = this.getAFalseSuccessor() and + ( + va = this.(NotExpr).getOperand() or + va = any(EQExpr eq | eq = this and eq.getAnOperand().getValue() = "0").getAnOperand() or + va = getCheckedFalseCondition(this) or + va = + any(NEExpr eq | eq = getCheckedFalseCondition(this) and eq.getAnOperand().getValue() = "0") + .getAnOperand() + ) + or + nullSuccessor = this.getAFalseSuccessor() and + notNullSuccessor = this.getATrueSuccessor() and + ( + va = this or + va = any(NEExpr eq | eq = this and eq.getAnOperand().getValue() = "0").getAnOperand() or + va = + any(EQExpr eq | eq = getCheckedFalseCondition(this) and eq.getAnOperand().getValue() = "0") + .getAnOperand() + ) + ) + } + + /** The parameter being null-checked. */ + Parameter getParameter() { result = p } + + override ControlFlowNode getIgnoredSuccessorForContext(Context c) { + c = ParamNull(p) and result = notNullSuccessor + or + c = ParamNotNull(p) and result = nullSuccessor + } + + /** The successor at which the parameter is confirmed to be null. */ + ControlFlowNode getNullSuccessor() { result = nullSuccessor } + + /** The successor at which the parameter is confirmed to be not-null. */ + ControlFlowNode getNotNullSuccessor() { result = notNullSuccessor } +} + +/** + * An entry in a CSV file in cond-init that contains externally defined functions that are + * conditional initializers. These files are typically produced by running the + * ConditionallyInitializedFunction companion query. + */ +class ValidatedExternalCondInitFunction extends ExternalData { + ValidatedExternalCondInitFunction() { this.getDataPath().matches("%cond-init%.csv") } + + predicate isExternallyVerified(Function f, int param) { + functionSignature(f, this.getField(1), this.getField(2)) and param = this.getFieldAsInt(3) + } +} + +/** + * The type of evidence used to determine whether a function initializes a parameter. + */ +newtype Evidence = + /** + * The function is defined in the snapshot, and the CFG has been analyzed to determine that the + * parameter is not initialized on at least one path to the exit. + */ + DefinitionInSnapshot() or + /** + * The function is externally defined, but the parameter has an `_out` SAL annotation which + * suggests that it is initialized in the function. + */ + SuggestiveSalAnnotation() or + /** + * We have been given a CSV file which indicates this parameter is conditionally initialized. + */ + ExternalEvidence() + +/** + * A call to an function which initializes one or more of its parameters. + */ +class InitializationFunctionCall extends FunctionCall { + Expr initializedArgument; + + InitializationFunctionCall() { initializedArgument = getAnInitializedArgument(this) } + + /** Gets a parameter that is initialized by this call. */ + Parameter getAnInitParameter() { result.getAnAccess() = initializedArgument } +} + +/** + * A variable access which is dereferenced then assigned to. + */ +private predicate isPointerDereferenceAssignmentTarget(VariableAccess target) { + target.getParent().(PointerDereferenceExpr) = any(Assignment e).getLValue() +} + +/** + * A function which initializes one or more of its parameters. + */ +class InitializationFunction extends Function { + int i; + Evidence evidence; + + InitializationFunction() { + evidence = DefinitionInSnapshot() and + ( + // Assignment by pointer dereferencing the parameter + isPointerDereferenceAssignmentTarget(this.getParameter(i).getAnAccess()) or + // Field wise assignment to the parameter + any(Assignment e).getLValue() = getAFieldAccess(this.getParameter(i)) or + i = + this.(MemberFunction) + .getAnOverridingFunction+() + .(InitializationFunction) + .initializedParameter() or + this.getParameter(i) = any(InitializationFunctionCall c).getAnInitParameter() + ) + or + // We have some external information that this function conditionally initializes + not this.hasDefinition() and + any(ValidatedExternalCondInitFunction vc).isExternallyVerified(this, i) and + evidence = ExternalEvidence() + } + + /** Gets a parameter index which is initialized by this function. */ + int initializedParameter() { result = i } + + /** Gets a `ControlFlowNode` which assigns a new value to the parameter with the given index. */ + ControlFlowNode paramReassignment(int index) { + index = i and + ( + result = this.getParameter(i).getAnAccess() and + ( + result = any(Assignment a).getLValue().(PointerDereferenceExpr).getOperand() + or + // Field wise assignment to the parameter + result = any(Assignment a).getLValue().(FieldAccess).getQualifier() + or + // Assignment to a nested field of the parameter + result = any(Assignment a).getLValue().(NestedFieldAccess).getUltimateQualifier() + or + result = getAnInitializedArgument(any(Call c)) + or + exists(IfStmt check | result = check.getCondition().getAChild*() | + this.paramReassignmentCondition(check) + ) + ) + or + result = + any(AssumeExpr e | + e.getEnclosingFunction() = this and e.getAChild().(Literal).getValue() = "0" + ) + ) + } + + /** + * Helper predicate: holds if the `if` statement `check` contains a + * reassignment to the `i`th parameter within its `then` statement. + */ + pragma[noinline] + private predicate paramReassignmentCondition(IfStmt check) { + this.paramReassignment(i).getEnclosingStmt().getParentStmt*() = check.getThen() + } + + /** Holds if `n` can be reached without the parameter at `index` being reassigned. */ + predicate paramNotReassignedAt(ControlFlowNode n, int index, Context c) { + c = this.getAContext(index) and + ( + not exists(this.getEntryPoint()) and index = i and n = this + or + n = this.getEntryPoint() and index = i + or + exists(ControlFlowNode mid | this.paramNotReassignedAt(mid, index, c) | + n = mid.getASuccessor() and + not n = this.paramReassignment(index) and + /* + * Ignore successor edges where the parameter is null, because it is then confirmed to be + * initialized. + */ + + not exists(ParameterNullCheck nullCheck | + nullCheck = mid and + nullCheck = this.getANullCheck(index) and + n = nullCheck.getNullSuccessor() + ) and + /* + * Ignore successor edges which are excluded by the given context + */ + + not exists(ParameterCheck paramCheck | paramCheck = mid | + n = paramCheck.getIgnoredSuccessorForContext(c) + ) + ) + ) + } + + /** Gets a null-check on the parameter at `index`. */ + private ParameterNullCheck getANullCheck(int index) { + this.getParameter(index) = result.getParameter() + } + + /** Gets a parameter which is not at the given index. */ + private Parameter getOtherParameter(int index) { + index = i and + result = this.getAParameter() and + not result.getIndex() = index + } + + /** + * Gets a call `Context` that is applicable when considering whether parameter at the `index` can + * be conditionally initialized. + */ + Context getAContext(int index) { + index = i and + /* + * If there is one and only one other parameter which is null checked in the body of the method, + * then we have two contexts to consider - that the other param is null, or that the other param + * is not null. + */ + + if + strictcount(Parameter p | + exists(Context c | c = ParamNull(p) or c = ParamNotNull(p)) and + p = this.getOtherParameter(index) + ) = 1 + then + exists(Parameter p | p = this.getOtherParameter(index) | + result = ParamNull(p) or result = ParamNotNull(p) + ) + else + // Otherwise, only consider NoContext. + result = NoContext() + } + + /** + * Holds if this function should be whitelisted - that is, not considered as conditionally + * initializing its parameters. + */ + predicate whitelisted() { + exists(string name | this.hasName(name) | + // Return value is not a success code but the output functions never fail. + name.matches("_Interlocked%") + or + name = + [ + // Functions that never fail, according to MSDN. + "QueryPerformanceCounter", "QueryPerformanceFrequency", + // Functions that never fail post-Vista, according to MSDN. + "InitializeCriticalSectionAndSpinCount", + // `rand_s` writes 0 to a non-null argument if it fails, according to MSDN. + "rand_s", + // IntersectRect initializes the argument regardless of whether the input intersects + "IntersectRect", "SetRect", "UnionRect", + // These functions appears to have an incorrect CFG, which leads to false positives + "PhysicalToLogicalDPIPoint", "LogicalToPhysicalDPIPoint", + // Sets NtProductType to default on error + "RtlGetNtProductType", + // Our CFG is not sophisticated enough to detect that the argument is always initialized + "StringCchLengthA", + // All paths init the argument, and always returns SUCCESS. + "RtlUnicodeToMultiByteSize", + // All paths init the argument, and always returns SUCCESS. + "RtlMultiByteToUnicodeSize", + // All paths init the argument, and always returns SUCCESS. + "RtlUnicodeToMultiByteN", + // Always initializes argument + "RtlGetFirstRange", + // Destination range is zeroed out on failure, assuming first two parameters are valid + "memcpy_s", + // This zeroes the memory unconditionally + "SeCreateAccessState", + // Argument initialization is optional, but always succeeds + "KeGetCurrentProcessorNumberEx" + ] + ) + } +} + +/** + * A function which initializes one or more of its parameters, but not on all paths. + */ +class ConditionalInitializationFunction extends InitializationFunction { + Context c; + + ConditionalInitializationFunction() { + c = this.getAContext(i) and + not this.whitelisted() and + exists(Type status | status = this.getType().getUnspecifiedType() | + status instanceof IntegralType or + status instanceof Enum + ) and + not this.getType().getName().toLowerCase() = "size_t" and + ( + /* + * If there is no definition, consider this to be conditionally initializing (based on either + * SAL or external data). + */ + + not evidence = DefinitionInSnapshot() + or + /* + * If this function is defined in this snapshot, then it conditionally initializes if there + * is at least one path through the function which doesn't initialize the parameter. + * + * Explicitly ignore pure virtual functions. + */ + + this.hasDefinition() and + this.paramNotReassignedAt(this, i, c) and + not this instanceof PureVirtualFunction + ) + } + + /** Gets the evidence associated with the given parameter. */ + Evidence getEvidence(int param) { + /* + * Note: due to the way the predicate dispatch interacts with fields, this needs to be + * implemented on this class, not `InitializationFunction`. If implemented on the latter it + * can return evidence that does not result in conditional initialization. + */ + + param = i and evidence = result + } + + /** Gets the index of a parameter which is conditionally initialized. */ + int conditionallyInitializedParameter(Context context) { result = i and context = c } +} + +/** + * More elaborate tracking, flagging cases where the status is checked after + * the potentially uninitialized variable has been used, and ignoring cases + * where the status is not checked but there is no use of the potentially + * uninitialized variable, may be obtained via `getARiskyAccess`. + */ +class ConditionalInitializationCall extends FunctionCall { + ConditionalInitializationFunction target; + + ConditionalInitializationCall() { target = getTarget(this) } + + /** Gets the argument passed for the given parameter to this call. */ + Expr getArgumentForParameter(Parameter p) { + p = this.getTarget().getAParameter() and + result = this.getArgument(p.getIndex()) + } + + /** + * Gets an argument conditionally initialized by this call. + */ + Expr getAConditionallyInitializedArgument(ConditionalInitializationFunction condTarget, Evidence e) { + condTarget = target and + exists(Context context | + result = getAConditionallyInitializedArgument(this, condTarget, context, e) + | + context = NoContext() + or + exists(Parameter otherP, Expr otherArg | + context = ParamNotNull(otherP) or + context = ParamNull(otherP) + | + otherArg = this.getArgumentForParameter(otherP) and + (otherArg instanceof AddressOfExpr implies context = ParamNotNull(otherP)) and + (otherArg.getType() instanceof ArrayType implies context = ParamNotNull(otherP)) and + (otherArg.getValue() = "0" implies context = ParamNull(otherP)) + ) + ) + } + + VariableAccess getAConditionallyInitializedVariable() { + not result.getTarget().getAnAssignedValue().getASuccessor+() = result and + // Should not be assigned field-wise prior to the call. + not exists(Assignment a, FieldAccess fa | + fa.getQualifier() = result.getTarget().getAnAccess() and + a.getLValue() = fa and + fa.getASuccessor+() = result + ) and + result = + this.getArgument(getTarget(this) + .(ConditionalInitializationFunction) + .conditionallyInitializedParameter(_)).(AddressOfExpr).getOperand() + } + + Variable getStatusVariable() { + exists(AssignExpr a | a.getLValue() = result.getAnAccess() | a.getRValue() = this) + or + result.getInitializer().getExpr() = this + } + + Expr getSuccessCheck() { + exists(this.getAFalseSuccessor()) and result = this + or + result = this.getParent() and + ( + result instanceof NotExpr or + result.(EQExpr).getAnOperand().getValue() = "0" or + result.(GEExpr).getLesserOperand().getValue() = "0" + ) + } + + Expr getFailureCheck() { + result = this.getParent() and + ( + result instanceof NotExpr or + result.(NEExpr).getAnOperand().getValue() = "0" or + result.(LTExpr).getLesserOperand().getValue() = "0" + ) + } + + private predicate inCheckedContext() { + exists(Call parent | this = parent.getAnArgument() | + parent.getTarget() instanceof Operator or + parent.getTarget().hasName("VerifyOkCatastrophic") + ) + } + + ControlFlowNode uncheckedReaches(LocalVariable var) { + ( + not exists(var.getInitializer()) and + var = this.getAConditionallyInitializedVariable().getTarget() and + if exists(this.getFailureCheck()) + then result = this.getFailureCheck().getATrueSuccessor() + else + if exists(this.getSuccessCheck()) + then result = this.getSuccessCheck().getAFalseSuccessor() + else ( + result = this.getASuccessor() and not this.inCheckedContext() + ) + ) + or + exists(ControlFlowNode mid | mid = this.uncheckedReaches(var) | + not mid = this.getStatusVariable().getAnAccess() and + not mid = var.getAnAccess() and + not exists(VariableAccess write | result = write and write = var.getAnAccess() | + write = any(AssignExpr a).getLValue() or + write = any(AddressOfExpr a).getOperand() + ) and + result = mid.getASuccessor() + ) + } + + VariableAccess getARiskyRead(Function f) { + f = this.getTarget() and + exists(this.getFile().getRelativePath()) and + result = this.uncheckedReaches(result.getTarget()) and + not this.(GuardCondition).controls(result.getBasicBlock(), _) + } +} + +/** + * Gets the position of an argument to the call which is initialized by the call. + */ +pragma[nomagic] +int initializedArgument(Call call) { + exists(InitializationFunction target | + target = getTarget(call) and + result = target.initializedParameter() + ) +} + +/** + * Gets an argument which is initialized by the call. + */ +Expr getAnInitializedArgument(Call call) { result = call.getArgument(initializedArgument(call)) } + +/** + * Gets the position of an argument to the call to the target which is conditionally initialized by + * the call, under the given context and evidence. + */ +pragma[nomagic] +private int conditionallyInitializedArgument( + Call call, ConditionalInitializationFunction target, Context c, Evidence e +) { + target = getTarget(call) and + c = target.getAContext(result) and + e = target.getEvidence(result) and + result = target.conditionallyInitializedParameter(c) +} + +/** + * Gets an argument which is conditionally initialized by the call to the given target under the given context and evidence. + */ +Expr getAConditionallyInitializedArgument( + Call call, ConditionalInitializationFunction target, Context c, Evidence e +) { + result = call.getArgument(conditionallyInitializedArgument(call, target, c, e)) +} + +/** + * Gets the type signature for the functions parameters. + */ +private string typeSig(Function f) { + result = + concat(int i, Type pt | + pt = f.getParameter(i).getType() + | + pt.getUnspecifiedType().toString(), "," order by i + ) +} + +/** + * Holds where qualifiedName and typeSig make up the signature for the function. + */ +private predicate functionSignature(Function f, string qualifiedName, string typeSig) { + qualifiedName = f.getQualifiedName() and + typeSig = typeSig(f) +} + +/** + * Gets a possible definition for the undefined function by matching the undefined function name + * and parameter arity with a defined function. + * + * This is useful for identifying call to target dependencies across libraries, where the libraries + * are never statically linked together. + */ +private Function getAPossibleDefinition(Function undefinedFunction) { + not undefinedFunction.hasDefinition() and + exists(string qn, string typeSig | + functionSignature(undefinedFunction, qn, typeSig) and functionSignature(result, qn, typeSig) + ) and + result.hasDefinition() +} + +/** + * Helper predicate for `getTarget`, that computes possible targets of a `Call`. + * + * If there is at least one defined target after performing some simple virtual dispatch + * resolution, then the result is all the defined targets. + */ +private Function getTarget1(Call c) { + result = VirtualDispatch::getAViableTarget(c) and + result.hasDefinition() +} + +/** + * Helper predicate for `getTarget`, that computes possible targets of a `Call`. + * + * If we can use the heuristic matching of functions to find definitions for some of the viable + * targets, return those. + */ +private Function getTarget2(Call c) { + not exists(getTarget1(c)) and + result = getAPossibleDefinition(VirtualDispatch::getAViableTarget(c)) +} + +/** + * Helper predicate for `getTarget`, that computes possible targets of a `Call`. + * + * Otherwise, the result is the undefined `Function` instances. + */ +private Function getTarget3(Call c) { + not exists(getTarget1(c)) and + not exists(getTarget2(c)) and + result = VirtualDispatch::getAViableTarget(c) +} + +/** + * Gets a possible target for the `Call`, using the name and parameter matching if we did not associate + * this call with a specific definition at link or compile time, and performing simple virtual + * dispatch resolution. + */ +Function getTarget(Call c) { + result = getTarget1(c) or + result = getTarget2(c) or + result = getTarget3(c) +} + +/** + * Get an access of a field on `Variable` v. + */ +FieldAccess getAFieldAccess(Variable v) { + exists(VariableAccess va, Expr qualifierExpr | + // Find an access of the variable, or an AddressOfExpr that has the access + va = v.getAnAccess() and + ( + qualifierExpr = va or + qualifierExpr.(AddressOfExpr).getOperand() = va + ) + | + // Direct field access + qualifierExpr = result.getQualifier() + or + // Nested field access + qualifierExpr = result.(NestedFieldAccess).getUltimateQualifier() + ) +} + +/** + * Gets a condition which is checked to be false by the given `ne` expression, according to this pattern: + * ``` + * int a = !!result; + * if (!a) { // <- ne + * .... + * } + * ``` + */ +private Expr getCheckedFalseCondition(NotExpr ne) { + exists(LocalVariable v | + result = v.getInitializer().getExpr().(NotExpr).getOperand().(NotExpr).getOperand() and + ne.getOperand() = v.getAnAccess() and + nonAssignedVariable(v) + // and not passed by val? + ) +} + +pragma[noinline] +private predicate nonAssignedVariable(Variable v) { not exists(v.getAnAssignment()) } diff --git a/cpp/common/src/codingstandards/cpp/rules/readofuninitializedmemory/ReadOfUninitializedMemory.qll b/cpp/common/src/codingstandards/cpp/rules/readofuninitializedmemory/ReadOfUninitializedMemory.qll index 8d701cb26c..8a74cc2e2e 100644 --- a/cpp/common/src/codingstandards/cpp/rules/readofuninitializedmemory/ReadOfUninitializedMemory.qll +++ b/cpp/common/src/codingstandards/cpp/rules/readofuninitializedmemory/ReadOfUninitializedMemory.qll @@ -7,6 +7,7 @@ import codingstandards.cpp.Customizations import codingstandards.cpp.Exclusions import semmle.code.cpp.controlflow.Guards import semmle.code.cpp.controlflow.SubBasicBlocks +import InitializationFunctions abstract class ReadOfUninitializedMemorySharedQuery extends Query { } @@ -121,13 +122,39 @@ class InitializationContext extends TInitializationContext { } } +class NewNotInit extends NewExpr { + NewNotInit() { + this.getAllocatedType() instanceof BuiltInType and + not exists(this.getAChild()) + } +} + +class NonInitAssignment extends Assignment { + NonInitAssignment() { this.getRValue() instanceof NewNotInit } +} + +class VariableAccessOnLHSOfNonInitAssignment extends VariableAccess { + VariableAccessOnLHSOfNonInitAssignment() { exists(NonInitAssignment a | this = a.getLValue()) } +} + /** * A local variable without an initializer which is amenable to initialization analysis. */ class UninitializedVariable extends LocalVariable { UninitializedVariable() { // Not initialized at declaration - not exists(getInitializer()) and + ( + not exists(getInitializer().getExpr()) + or + //or is a builtin type used with new operator but there is no value initialization as far as we can see + exists(Initializer i, NewExpr n | + i = getInitializer() and + n = i.getExpr() and + this.getUnspecifiedType().stripType() instanceof BuiltInType and + //ignore value init + not exists(n.getAChild()) + ) + ) and // Not static or thread local, because they are not initialized with indeterminate values not isStatic() and not isThreadLocal() and @@ -176,16 +203,33 @@ class UninitializedVariable extends LocalVariable { ) } - /** Get a access of the variable that is used to initialize the variable. */ + /** + * Get a access of the variable that is assumed to initialize the variable. + * This approximates that any access in the lvalue category may be a definition. + */ VariableAccess getADefinitionAccess() { result = getAnAccess() and - result.isLValueCategory() + result.isLValueCategory() and + // Not a pointless read + not result = any(ExprStmt es).getExpr() and + // not involved in a new expr assignment since that does not define + not result instanceof VariableAccessOnLHSOfNonInitAssignment } - /** Get a read of the variable. */ + /** + * Gets an access of the variable `v` which is not used as an lvalue, and not used as an argument + * to an initialization function. + */ VariableAccess getAUse() { - result = getAnAccess() and - result.isRValueCategory() and + result = this.getAnAccess() and + // Not used as an lvalue + not result = any(AssignExpr a).getLValue() and + // Not passed to another initialization function + not exists(Call c, int j | j = c.getTarget().(InitializationFunction).initializedParameter() | + result = c.getArgument(j).(AddressOfExpr).getOperand() + ) and + // Not a pointless read + not result = any(ExprStmt es).getExpr() and // sizeof operators are not real uses not result.getParent+() instanceof SizeofOperator } diff --git a/cpp/common/test/rules/donotcopyaddressofautostorageobjecttootherobject/DoNotCopyAddressOfAutoStorageObjectToOtherObject.expected b/cpp/common/test/rules/donotcopyaddressofautostorageobjecttootherobject/DoNotCopyAddressOfAutoStorageObjectToOtherObject.expected index 8699ae2d8a..35d2fd438a 100644 --- a/cpp/common/test/rules/donotcopyaddressofautostorageobjecttootherobject/DoNotCopyAddressOfAutoStorageObjectToOtherObject.expected +++ b/cpp/common/test/rules/donotcopyaddressofautostorageobjecttootherobject/DoNotCopyAddressOfAutoStorageObjectToOtherObject.expected @@ -62,3 +62,5 @@ | stack_escapes_test.cpp:367:3:367:17 | ... = ... | A stack address ($@) may be assigned to a non-local variable. | stack_escapes_test.cpp:367:17:367:17 | x | source | | stack_escapes_test.cpp:368:3:368:20 | ... = ... | A stack address ($@) may be assigned to a non-local variable. | stack_escapes_test.cpp:368:20:368:20 | x | source | | test.cpp:7:5:7:10 | ... = ... | A stack address ($@) may be assigned to a non-local variable. | test.cpp:7:10:7:10 | c | source | +| test.cpp:15:5:15:11 | ... = ... | A stack address ($@) may be assigned to a non-local variable. | test.cpp:15:9:15:11 | l_a | source | +| test.cpp:26:5:26:10 | ... = ... | A stack address ($@) may be assigned to a non-local variable. | test.cpp:25:11:25:11 | l | source | diff --git a/cpp/common/test/rules/donotcopyaddressofautostorageobjecttootherobject/test.cpp b/cpp/common/test/rules/donotcopyaddressofautostorageobjecttootherobject/test.cpp index 0cd3970c7e..5014ecf9cb 100644 --- a/cpp/common/test/rules/donotcopyaddressofautostorageobjecttootherobject/test.cpp +++ b/cpp/common/test/rules/donotcopyaddressofautostorageobjecttootherobject/test.cpp @@ -6,4 +6,23 @@ void test_simple_aliasing() { int c; a = &c; // NON_COMPLIANT - different scope } +} + +void extra_test_simple_aliasing() { + int *p; + { + int l_a[1]; + p = l_a; // NON_COMPLIANT + } +} + +void extra_test2_simple_aliasing() { + int *p; + { + int *p2 = nullptr; + int l; + + p2 = &l; // COMPLIANT + p = p2; // NON_COMPLIANT + } } \ No newline at end of file diff --git a/cpp/common/test/rules/readofuninitializedmemory/ReadOfUninitializedMemory.expected b/cpp/common/test/rules/readofuninitializedmemory/ReadOfUninitializedMemory.expected index 2fb2b79a0f..5cfa1d51f6 100644 --- a/cpp/common/test/rules/readofuninitializedmemory/ReadOfUninitializedMemory.expected +++ b/cpp/common/test/rules/readofuninitializedmemory/ReadOfUninitializedMemory.expected @@ -3,3 +3,13 @@ | test.cpp:39:7:39:8 | l3 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:38:6:38:7 | l3 | l3 | | test.cpp:86:9:86:16 | arrayPtr | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:79:8:79:15 | arrayPtr | arrayPtr | | test.cpp:93:9:93:10 | l5 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:89:7:89:8 | l5 | l5 | +| test.cpp:134:11:134:11 | i | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:133:7:133:7 | i | i | +| test.cpp:137:13:137:14 | i1 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:136:8:136:9 | i1 | i1 | +| test.cpp:141:7:141:8 | i3 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:139:8:139:9 | i3 | i3 | +| test.cpp:141:13:141:14 | i1 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:136:8:136:9 | i1 | i1 | +| test.cpp:201:7:201:8 | p0 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:200:8:200:9 | p0 | p0 | +| test.cpp:204:4:204:5 | p1 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:203:8:203:9 | p1 | p1 | +| test.cpp:206:7:206:8 | p1 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:203:8:203:9 | p1 | p1 | +| test.cpp:211:7:211:8 | p2 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:209:8:209:9 | p2 | p2 | +| test.cpp:214:10:214:11 | p2 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:209:8:209:9 | p2 | p2 | +| test.cpp:221:7:221:8 | p4 | Local variable $@ is read here and may not be initialized on all paths. | test.cpp:219:8:219:9 | p4 | p4 | diff --git a/cpp/common/test/rules/readofuninitializedmemory/test.cpp b/cpp/common/test/rules/readofuninitializedmemory/test.cpp index 6ed07d795f..7d5600a19a 100644 --- a/cpp/common/test/rules/readofuninitializedmemory/test.cpp +++ b/cpp/common/test/rules/readofuninitializedmemory/test.cpp @@ -123,4 +123,101 @@ void test_non_default_init() { use(tlp); // COMPLIANT - thread local variables are zero initialized _Atomic int ai; use(ai); // COMPLIANT - atomics are special and not covered by this rule +} + +namespace { +int i; // COMPLIANT +} + +void extra_test() { + int i; + int j = i + 1; // NON_COMPLIANT + + int *i1 = new int; + int i2 = *i1; // NON_COMPLIANT + + int *i3; + + if (i3 == i1) { // NON_COMPLIANT + } +} + +void extra_conditionals(bool b) { + if (b) { + goto L; + } + int i; + i = 1; +L: + i = i + 1; // NON_COMPLIANT[FALSE_NEGATIVE] +} + +struct S { + int m1; + int m2; +}; + +void struct_test() { + S s1; + S s2 = {1}; + + auto i1 = s1.m1; // NON_COMPLIANT[FALSE_NEGATIVE] - rule currently is not + // field sensitive + auto i2 = s2.m2; // COMPLIANT + + int a1[10] = {1, 1, 1}; + int a2[10]; + + auto a3 = a1[5]; // COMPLIANT + auto a4 = a2[5]; // NON_COMPLIANT[FALSE_NEGATIVE] +} + +class C { +private: + int m1; + int m2; + +public: + C() : m1(1), m2(1) {} + + C(int a) : m1(a) {} + + int getm2() { return m2; } +}; + +void test_class() { + C c1; + if (c1.getm2() > 0) { // COMPLIANT + } + + C c2(5); + if (c2.getm2() > 0) { // NON_COMPLIANT[FALSE_NEGATIVE] - rule currently is not + // field sensitive + } +} + +void extra_extra_test() { + int *p0 = new int; + use(p0); // NON_COMPLIANT + + int *p1 = new int; + *p1 = 0; // COMPLIANT[FALSE_POSITIVE] -- this is not found bc this is not an + // lvalue access + use(p1); // COMPLIANT[FALSE_POSITIVE] -- the pointee of p1 has been + // initialized + + int *p2 = new int; + p2 = new int; + use(p2); // NON_COMPLIANT + + int *p3 = new int(1); + *p3 = *p2; // NON_COMPLIANT -- the pointee of p2 has not been + // initialized + use(p3); // NON_COMPLIANT[FALSE_NEGATIVE] -- the pointee of p3 has be + // overridden + + int *p4; + p4 = new int; + use(p4); // NON_COMPLIANT -- the pointee of p4 has not been + // initialized } \ No newline at end of file diff --git a/cpp/misra/src/rules/RULE-11-6-2/ValueOfAnObjectMustNotBeReadBeforeItHasBeenSet.ql b/cpp/misra/src/rules/RULE-11-6-2/ValueOfAnObjectMustNotBeReadBeforeItHasBeenSet.ql new file mode 100644 index 0000000000..3b1ab81c2e --- /dev/null +++ b/cpp/misra/src/rules/RULE-11-6-2/ValueOfAnObjectMustNotBeReadBeforeItHasBeenSet.ql @@ -0,0 +1,25 @@ +/** + * @id cpp/misra/value-of-an-object-must-not-be-read-before-it-has-been-set + * @name RULE-11-6-2: The value of an object must not be read before it has been set + * @description Reading from uninitialized indeterminate values may produce undefined behavior. + * @kind problem + * @precision medium + * @problem.severity error + * @tags external/misra/id/rule-11-6-2 + * correctness + * security + * scope/system + * external/misra/enforcement/undecidable + * external/misra/obligation/mandatory + */ + +import cpp +import codingstandards.cpp.misra +import codingstandards.cpp.rules.readofuninitializedmemory.ReadOfUninitializedMemory + +class ValueOfAnObjectMustNotBeReadBeforeItHasBeenSetQuery extends ReadOfUninitializedMemorySharedQuery +{ + ValueOfAnObjectMustNotBeReadBeforeItHasBeenSetQuery() { + this = LifetimePackage::valueOfAnObjectMustNotBeReadBeforeItHasBeenSetQuery() + } +} diff --git a/cpp/misra/src/rules/RULE-6-8-3/AutomaticStorageAssignedToObjectGreaterLifetime.ql b/cpp/misra/src/rules/RULE-6-8-3/AutomaticStorageAssignedToObjectGreaterLifetime.ql new file mode 100644 index 0000000000..fe6911e35b --- /dev/null +++ b/cpp/misra/src/rules/RULE-6-8-3/AutomaticStorageAssignedToObjectGreaterLifetime.ql @@ -0,0 +1,26 @@ +/** + * @id cpp/misra/automatic-storage-assigned-to-object-greater-lifetime + * @name RULE-6-8-3: Declare objects with appropriate storage durations + * @description When storage durations are not compatible between assigned pointers it can lead to + * referring to objects outside of their lifetime, which is undefined behaviour. + * @kind problem + * @precision very-high + * @problem.severity error + * @tags external/misra/id/rule-6-8-3 + * correctness + * security + * scope/single-translation-unit + * external/misra/enforcement/decidable + * external/misra/obligation/required + */ + +import cpp +import codingstandards.cpp.misra +import codingstandards.cpp.rules.donotcopyaddressofautostorageobjecttootherobject.DoNotCopyAddressOfAutoStorageObjectToOtherObject + +class AutomaticStorageAssignedToObjectGreaterLifetimeConfig extends DoNotCopyAddressOfAutoStorageObjectToOtherObjectSharedQuery +{ + AutomaticStorageAssignedToObjectGreaterLifetimeConfig() { + this = LifetimePackage::automaticStorageAssignedToObjectGreaterLifetimeQuery() + } +} diff --git a/cpp/misra/test/rules/RULE-11-6-2/ValueOfAnObjectMustNotBeReadBeforeItHasBeenSet.testref b/cpp/misra/test/rules/RULE-11-6-2/ValueOfAnObjectMustNotBeReadBeforeItHasBeenSet.testref new file mode 100644 index 0000000000..e2ec5838e7 --- /dev/null +++ b/cpp/misra/test/rules/RULE-11-6-2/ValueOfAnObjectMustNotBeReadBeforeItHasBeenSet.testref @@ -0,0 +1 @@ +cpp/common/test/rules/readofuninitializedmemory/ReadOfUninitializedMemory.ql \ No newline at end of file diff --git a/cpp/misra/test/rules/RULE-6-8-3/AutomaticStorageAssignedToObjectGreaterLifetime.testref b/cpp/misra/test/rules/RULE-6-8-3/AutomaticStorageAssignedToObjectGreaterLifetime.testref new file mode 100644 index 0000000000..fc0808753a --- /dev/null +++ b/cpp/misra/test/rules/RULE-6-8-3/AutomaticStorageAssignedToObjectGreaterLifetime.testref @@ -0,0 +1 @@ +cpp/common/test/rules/donotcopyaddressofautostorageobjecttootherobject/DoNotCopyAddressOfAutoStorageObjectToOtherObject.ql \ No newline at end of file diff --git a/rule_packages/cpp/Lifetime.json b/rule_packages/cpp/Lifetime.json new file mode 100644 index 0000000000..ea5d9e73c3 --- /dev/null +++ b/rule_packages/cpp/Lifetime.json @@ -0,0 +1,53 @@ +{ + "MISRA-C++-2023": { + "RULE-11-6-2": { + "properties": { + "enforcement": "undecidable", + "obligation": "mandatory" + }, + "queries": [ + { + "description": "Reading from uninitialized indeterminate values may produce undefined behavior.", + "kind": "problem", + "name": "The value of an object must not be read before it has been set", + "precision": "medium", + "severity": "error", + "short_name": "ValueOfAnObjectMustNotBeReadBeforeItHasBeenSet", + "shared_implementation_short_name": "ReadOfUninitializedMemory", + "tags": [ + "correctness", + "security", + "scope/system" + ] + } + ], + "title": "The value of an object must not be read before it has been set" + }, + "RULE-6-8-3": { + "properties": { + "enforcement": "decidable", + "obligation": "required" + }, + "queries": [ + { + "description": "When storage durations are not compatible between assigned pointers it can lead to referring to objects outside of their lifetime, which is undefined behaviour.", + "kind": "problem", + "name": "Declare objects with appropriate storage durations", + "precision": "very-high", + "severity": "error", + "short_name": "AutomaticStorageAssignedToObjectGreaterLifetime", + "shared_implementation_short_name": "DoNotCopyAddressOfAutoStorageObjectToOtherObject", + "tags": [ + "correctness", + "security", + "scope/single-translation-unit" + ], + "implementation_scope": { + "description": "The rule checks specifically for pointers to objects with automatic storage duration that are assigned to static storage duration variables." + } + } + ], + "title": "An assignment operator shall not assign the address of an object with automatic storage duration to an object with a greater lifetime" + } + } +} \ No newline at end of file diff --git a/rules.csv b/rules.csv index 6f2f204789..bb6d86ef3b 100644 --- a/rules.csv +++ b/rules.csv @@ -925,7 +925,7 @@ cpp,MISRA-C++-2023,RULE-10-4-1,Yes,Required,Decidable,Single Translation Unit,Th cpp,MISRA-C++-2023,RULE-11-3-1,Yes,Advisory,Decidable,Single Translation Unit,Variables of array type should not be declared,,Declarations2,Easy, cpp,MISRA-C++-2023,RULE-11-3-2,Yes,Advisory,Decidable,Single Translation Unit,The declaration of an object should contain no more than two levels of pointer indirection,A5-0-3,ImportMisra23,Import, cpp,MISRA-C++-2023,RULE-11-6-1,Yes,Advisory,Decidable,Single Translation Unit,All variables should be initialized,,Declarations2,Easy, -cpp,MISRA-C++-2023,RULE-11-6-2,Yes,Mandatory,Undecidable,System,The value of an object must not be read before it has been set,A8-5-0,Lifetime,Very Hard, +cpp,MISRA-C++-2023,RULE-11-6-2,Yes,Mandatory,Undecidable,System,The value of an object must not be read before it has been set,A8-5-0,Lifetime,Import cpp,MISRA-C++-2023,RULE-11-6-3,Yes,Required,Decidable,Single Translation Unit,"Within an enumerator list, the value of an implicitly-specified enumeration constant shall be unique",RULE-8-12,ImportMisra23,Import, cpp,MISRA-C++-2023,RULE-12-2-1,Yes,Advisory,Decidable,Single Translation Unit,Bit-fields should not be declared,A9-6-2,Banned,Easy, cpp,MISRA-C++-2023,RULE-12-2-2,Yes,Required,Decidable,Single Translation Unit,A bit-field shall have an appropriate type,RULE-6-1,ImportMisra23,Import,