diff --git a/cpp/ql/src/change-notes/2026-11-12-assignment-helper-return.md b/cpp/ql/src/change-notes/2026-11-12-assignment-helper-return.md new file mode 100644 index 000000000000..0e20ee12751a --- /dev/null +++ b/cpp/ql/src/change-notes/2026-11-12-assignment-helper-return.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `cpp/assignment-does-not-return-this` query now recognizes assignment operators that return `*this` through non-virtual helper functions whose reachable returns directly return the same reference parameter, reducing false positives. diff --git a/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql b/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql index 67559557c3cc..43119f179d5e 100644 --- a/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql +++ b/cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql @@ -17,54 +17,114 @@ import cpp * Applies to all assignment operators, not just the copy assignment operator. */ -predicate callOnThis(FunctionCall fc) { - // `this->f(...)` - fc.getQualifier() instanceof ThisExpr - or - // `(*this).f(...)` - fc.getQualifier().(PointerDereferenceExpr).getChild(0) instanceof ThisExpr +/** Holds if `e` has an explicit cast anywhere in its conversion chain. */ +private predicate hasExplicitCast(Expr e) { + exists(Cast c | c = e.getUnconverted().getConversion*() and not c.isImplicit()) } -predicate pointerThis(Expr e) { - e instanceof ThisExpr - or - // `f(...)` - // (includes `this = ...`, where `=` is overloaded so a `FunctionCall`) - exists(FunctionCall fc | fc = e and callOnThis(fc) | returnsPointerThis(fc.getTarget())) - or - // `this = ...` (where `=` is not overloaded, so an `AssignExpr`) - pointerThis(e.(AssignExpr).getLValue()) +predicate callOnThis(FunctionCall fc, boolean allowCasts) { + allowCasts = [true, false] and + (allowCasts = true or not hasExplicitCast(fc.getQualifier())) and + ( + // `this->f(...)` + fc.getQualifier() instanceof ThisExpr + or + // `(*this).f(...)` + fc.getQualifier().(PointerDereferenceExpr).getChild(0) instanceof ThisExpr and + ( + allowCasts = true or + not hasExplicitCast(fc.getQualifier().(PointerDereferenceExpr).getChild(0)) + ) + ) +} + +predicate pointerThis(Expr e, boolean allowCasts) { + allowCasts = [true, false] and + (allowCasts = true or not hasExplicitCast(e)) and + ( + e instanceof ThisExpr + or + // `f(...)` + // (includes `this = ...`, where `=` is overloaded so a `FunctionCall`) + exists(FunctionCall fc | fc = e and callOnThis(fc, allowCasts) | + returnsPointerThis(fc.getTarget(), allowCasts) + ) + or + // `this = ...` (where `=` is not overloaded, so an `AssignExpr`) + pointerThis(e.(AssignExpr).getLValue(), allowCasts) + ) } -predicate dereferenceThis(Expr e) { - pointerThis(e.(PointerDereferenceExpr).getChild(0)) - or - // `f(...)` - // (includes `*this = ...`, where `=` is overloaded so a `FunctionCall`) - exists(FunctionCall fc | fc = e and callOnThis(fc) | returnsDereferenceThis(fc.getTarget())) - or - // `*this = ...` (where `=` is not overloaded, so an `AssignExpr`) - dereferenceThis(e.(AssignExpr).getLValue()) - or - // `e ? ... : ... ` - exists(ConditionalExpr cond | - cond = e and - dereferenceThis(cond.getThen()) and - dereferenceThis(cond.getElse()) +/** + * Holds if the function containing `p` returns a reference to `p` on every + * reachable return. Require a visible, non-virtual definition and matching + * reference base types to exclude copies and user-defined conversions. + */ +private predicate returnsReferenceParameter(Parameter p) { + exists(Function f | + f = p.getFunction() and + exists(f.getBlock()) and + not f.isVirtual() and + f.getType().getUnspecifiedType().(ReferenceType).getBaseType().getUnspecifiedType() = + p.getType().getUnspecifiedType().(ReferenceType).getBaseType().getUnspecifiedType() and + exists(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s)) and + forall(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s) | + s.getExpr().getUnconverted().(VariableAccess).getTarget() = p and + not hasExplicitCast(s.getExpr()) + ) + ) +} + +// Preserve the existing treatment of casts outside helper arguments. Within a +// helper argument, check conversions at every step that supplies the result. +predicate dereferenceThis(Expr e, boolean allowCasts) { + allowCasts = [true, false] and + (allowCasts = true or not hasExplicitCast(e)) and + ( + pointerThis(e.(PointerDereferenceExpr).getChild(0), allowCasts) + or + // `f(...)` + // (includes `*this = ...`, where `=` is overloaded so a `FunctionCall`) + exists(FunctionCall fc | fc = e and callOnThis(fc, allowCasts) | + returnsDereferenceThis(fc.getTarget(), allowCasts) + ) + or + // `helper(*this, ...)`, where every return directly returns the corresponding parameter. + exists(FunctionCall fc, Parameter p, Expr arg | + fc = e and + not hasExplicitCast(fc) and + fc.getTarget() = p.getFunction() and + returnsReferenceParameter(p) and + arg = fc.getArgument(p.getIndex()) and + arg.getUnconverted().getType().getUnspecifiedType() = + p.getType().getUnspecifiedType().(ReferenceType).getBaseType().getUnspecifiedType() and + dereferenceThis(arg, false) + ) + or + // `*this = ...` (where `=` is not overloaded, so an `AssignExpr`) + dereferenceThis(e.(AssignExpr).getLValue(), allowCasts) + or + // `e ? ... : ... ` + exists(ConditionalExpr cond | + cond = e and + dereferenceThis(cond.getThen(), allowCasts) and + dereferenceThis(cond.getElse(), allowCasts) + ) + or + // `..., ... ` + dereferenceThis(e.(CommaExpr).getRightOperand(), allowCasts) ) - or - // `..., ... ` - dereferenceThis(e.(CommaExpr).getRightOperand()) } /** * Holds if all `return` statements in `f` return `this`, possibly indirectly. * This includes functions whose body is not in the database. */ -predicate returnsPointerThis(Function f) { +predicate returnsPointerThis(Function f, boolean allowCasts) { + allowCasts = [true, false] and forall(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s) | // `return this` - pointerThis(s.getExpr()) + pointerThis(s.getExpr(), allowCasts) ) } @@ -73,10 +133,11 @@ predicate returnsPointerThis(Function f) { * possibly indirectly. This includes functions whose body is not in the * database. */ -predicate returnsDereferenceThis(Function f) { +predicate returnsDereferenceThis(Function f, boolean allowCasts) { + allowCasts = [true, false] and forall(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s) | // `return *this` - dereferenceThis(s.getExpr()) + dereferenceThis(s.getExpr(), allowCasts) ) } @@ -94,7 +155,7 @@ predicate assignOperatorWithWrongType(Operator op, string msg) { predicate assignOperatorWithWrongResult(Operator op, string msg) { op.hasName("operator=") and - not returnsDereferenceThis(op) and + not returnsDereferenceThis(op, true) and exists(op.getBlock()) and not op.getType() instanceof VoidType and not assignOperatorWithWrongType(op, _) and diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected index f84792905b8f..adc93b2f71d0 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected @@ -2,3 +2,20 @@ | AV Rule 82.cpp:24:8:24:16 | operator= | Assignment operator in class Bad2 should have return type Bad2&. Otherwise a copy is created at each call. | | AV Rule 82.cpp:63:29:63:29 | operator= | Assignment operator in class TemplateReturnAssignment does not return a reference to *this. | | AV Rule 82.cpp:199:52:199:52 | operator= | Assignment operator in class TemplatedAssignmentBad should have return type TemplatedAssignmentBad&. Otherwise a copy is created at each call. | +| casts.cpp:21:24:21:32 | operator= | Assignment operator in class BadHelperReturnCast does not return a reference to *this. | +| casts.cpp:27:30:27:38 | operator= | Assignment operator in class BadHelperReturnCStyleCast does not return a reference to *this. | +| casts.cpp:33:26:33:34 | operator= | Assignment operator in class BadHelperArgumentCast does not return a reference to *this. | +| casts.cpp:40:25:40:33 | operator= | Assignment operator in class BadHelperPointerCast does not return a reference to *this. | +| casts.cpp:47:29:47:37 | operator= | Assignment operator in class BadHelperConditionalCast does not return a reference to *this. | +| casts.cpp:54:23:54:31 | operator= | Assignment operator in class BadHelperCommaCast does not return a reference to *this. | +| casts.cpp:65:30:65:38 | operator= | Assignment operator in class BadHelperMemberReturnCast does not return a reference to *this. | +| casts.cpp:72:27:72:35 | operator= | Assignment operator in class BadHelperQualifierCast does not return a reference to *this. | +| casts.cpp:102:24:102:32 | operator= | Assignment operator in class BadHelperResultCast does not return a reference to *this. | +| helpers.cpp:92:13:92:21 | operator= | Assignment operator in class BadOther does not return a reference to *this. | +| helpers.cpp:103:16:103:24 | operator= | Assignment operator in class BadBranches does not return a reference to *this. | +| helpers.cpp:113:19:113:27 | operator= | Assignment operator in class BadConditional does not return a reference to *this. | +| helpers.cpp:121:22:121:30 | operator= | Assignment operator in class BadValueParameter does not return a reference to *this. | +| helpers.cpp:129:18:129:26 | operator= | Assignment operator in class BadValueResult should have return type BadValueResult&. Otherwise a copy is created at each call. | +| helpers.cpp:137:19:137:27 | operator= | Assignment operator in class BadMissingBody does not return a reference to *this. | +| helpers.cpp:147:15:147:23 | operator= | Assignment operator in class BadVirtual does not return a reference to *this. | +| helpers.cpp:164:18:164:26 | operator= | Assignment operator in class BadConversion does not return a reference to *this. | diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/casts.cpp b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/casts.cpp new file mode 100644 index 000000000000..a215c60f9979 --- /dev/null +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/casts.cpp @@ -0,0 +1,106 @@ +// Explicit conversions can select a different subobject of the same base type. +template struct LeftBase : Base {}; +template struct RightBase : Base {}; +template struct TwoBases : LeftBase, RightBase {}; + +template +T& identity(T& destination) { return (destination); } + +template +T& otherBase(T& destination) { + return static_cast(static_cast&>( + static_cast&>(static_cast&>(destination)))); +} + +template +T& otherBaseCStyle(T& destination) { + return (T&)(RightBase&)(TwoBases&)(LeftBase&)destination; +} + +struct BadHelperReturnCast { + BadHelperReturnCast& operator=(int) { // $ Alert + return otherBase(*this); + } +}; + +struct BadHelperReturnCStyleCast { + BadHelperReturnCStyleCast& operator=(int) { // $ Alert + return otherBaseCStyle(*this); + } +}; + +struct BadHelperArgumentCast { + BadHelperArgumentCast& operator=(int) { // $ Alert + return identity(static_cast(static_cast&>( + static_cast&>(static_cast&>(*this))))); + } +}; + +struct BadHelperPointerCast { + BadHelperPointerCast& operator=(int) { // $ Alert + return identity(*static_cast(static_cast*>( + static_cast*>(static_cast*>(this))))); + } +}; + +struct BadHelperConditionalCast { + BadHelperConditionalCast& operator=(bool flag) { // $ Alert + return identity(flag ? *this : static_cast(static_cast&>( + static_cast&>(static_cast&>(*this))))); + } +}; + +struct BadHelperCommaCast { + BadHelperCommaCast& operator=(int) { // $ Alert + return identity((0, static_cast(static_cast&>( + static_cast&>(static_cast&>(*this)))))); + } +}; + +struct BadHelperMemberReturnCast { + BadHelperMemberReturnCast* otherPointer() { + return static_cast(static_cast*>( + static_cast*>(static_cast*>(this)))); + } + BadHelperMemberReturnCast& operator=(int) { // $ Alert + return identity(*this->otherPointer()); + } +}; + +struct BadHelperQualifierCast { + BadHelperQualifierCast* selfPointer() { return this; } + BadHelperQualifierCast& operator=(int) { // $ Alert + return identity(*static_cast(static_cast*>( + static_cast*>(static_cast*>(this))))->selfPointer()); + } +}; + +struct GoodHelperArgumentPaths { + GoodHelperArgumentPaths* selfPointer() { return this; } + + GoodHelperArgumentPaths& operator=(int) { // GOOD: parentheses are not casts. + return identity(((*this))); + } + GoodHelperArgumentPaths& operator=(bool flag) { // GOOD + return identity(flag ? *this : *this); + } + GoodHelperArgumentPaths& operator=(char) { // GOOD + return identity((0, *this)); + } + GoodHelperArgumentPaths& operator=(long flag) { // GOOD: the condition does not supply the reference. + return identity(static_cast(flag) ? *this : *this); + } + GoodHelperArgumentPaths& operator=(unsigned int value) { // GOOD: neither does the left comma operand. + return identity((static_cast(value), *this)); + } + GoodHelperArgumentPaths& operator=(double) { // GOOD + return identity(*this->selfPointer()); + } +}; + +struct BadHelperResultCast { + BadHelperResultCast& operator=(int) { // $ Alert + return static_cast(static_cast&>( + static_cast&>(static_cast&>(identity(*this))))); + } +}; diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/helpers.cpp b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/helpers.cpp new file mode 100644 index 000000000000..b2438b58bedd --- /dev/null +++ b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/helpers.cpp @@ -0,0 +1,191 @@ +// Regression cases for github/codeql#7035. +template +T& assignFirst(T& destination, const T& source) { + destination.value = source.value; + return destination; +} + +struct GoodTemplate { + int value; + GoodTemplate& operator=(const GoodTemplate& other) { // GOOD + return assignFirst(*this, other); + } + GoodTemplate& operator=(GoodTemplate&& other) { // GOOD + return assignFirst(*this, other); + } +}; + +struct GoodFree; +GoodFree& assignFree(GoodFree& destination, const GoodFree& source); +struct GoodFree { + int value; + GoodFree& operator=(const GoodFree& other) { // GOOD + return assignFree(*this, other); + } +}; +GoodFree& assignFree(GoodFree& destination, const GoodFree& source) { + destination.value = source.value; + return destination; +} + +struct StaticHelper { + template + static T& assign(const T& source, T& destination) { + destination.value = source.value; + return destination; + } +}; +struct GoodSecondArgument { + int value; + GoodSecondArgument& operator=(const GoodSecondArgument& other) { // GOOD + return StaticHelper::assign(other, *this); + } +}; + +template +T& branching(T& destination, bool flag) { + if (flag) return destination; + destination.value = 0; + return destination; +} +struct GoodBranches { + int value; + GoodBranches& operator=(int other) { // GOOD + return branching(*this, other != 0); + } +}; + +template +T& unreachableOther(T& destination, T& other) { + if (1) return destination; + else return other; +} +struct GoodUnreachable { + int value; + GoodUnreachable& operator=(GoodUnreachable& other) { // GOOD + return unreachableOther(*this, other); + } +}; + +struct GoodTypedef { + typedef GoodTypedef& Reference; + static Reference identity(Reference destination) { return (destination); } + GoodTypedef& operator=(int) { // GOOD + return identity(*this); + } +}; + +template +T& maybeThrow(T& destination, bool fail) { + if (fail) throw 1; + return destination; +} +struct GoodThrows { + GoodThrows& operator=(int other) { // GOOD + return maybeThrow(*this, other != 0); + } +}; + +template +T& returnOther(T& destination, T& other) { return other; } +struct BadOther { + BadOther& operator=(BadOther& other) { // $ Alert + return returnOther(*this, other); + } +}; + +template +T& mixedReturns(T& destination, T& other, bool flag) { + if (flag) return destination; + return other; +} +struct BadBranches { + BadBranches& operator=(BadBranches& other) { // $ Alert + return mixedReturns(*this, other, false); + } +}; + +template +T& conditionalReturn(T& destination, T& other, bool flag) { + return flag ? destination : other; +} +struct BadConditional { + BadConditional& operator=(BadConditional& other) { // $ Alert + return conditionalReturn(*this, other, false); + } +}; + +template +T& byValue(T destination) { return destination; } +struct BadValueParameter { + BadValueParameter& operator=(int) { // $ Alert + return byValue(*this); + } +}; + +template +T byValueResult(T& destination) { return destination; } +struct BadValueResult { + BadValueResult operator=(int) { // $ Alert + return byValueResult(*this); + } +}; + +struct BadMissingBody; +BadMissingBody& unknownIdentity(BadMissingBody& destination); +struct BadMissingBody { + BadMissingBody& operator=(int) { // $ Alert + return unknownIdentity(*this); + } +}; + +struct BadVirtual; +struct VirtualHelper { + virtual BadVirtual& select(BadVirtual& destination) { return destination; } +}; +struct BadVirtual { + BadVirtual& operator=(VirtualHelper& helper) { // $ Alert + return helper.select(*this); + } +}; +struct OtherVirtualHelper : VirtualHelper { + BadVirtual other; + BadVirtual& select(BadVirtual&) override { return other; } +}; + +struct BadConversion; +struct Proxy { + operator BadConversion&(); +}; +BadConversion& convertingHelper(Proxy& proxy) { return proxy; } +struct BadConversion { + Proxy proxy; + operator Proxy&() { return proxy; } + BadConversion& operator=(int) { // $ Alert + return convertingHelper(*this); + } +}; + +void instantiateHelpers() { + GoodTemplate first, second; + first = second; + first = static_cast(second); + GoodSecondArgument a, b; + a = b; + GoodBranches branches; + branches = 1; + GoodUnreachable x, y; + x = y; + GoodThrows throwing; + throwing = 0; + BadOther o, p; + o = p; + BadBranches c, d; + c = d; + BadConditional e, f; + e = f; + BadValueParameter value; + value = 0; + BadValueResult result; + result = 0; +}