Repository navigation
C++: recognize assignment helpers returning reference parameters - #23109
SkyCipherr wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Explicit casts in helper returns and arguments can suppress genuine assignment alerts by selecting a different base subobject, and final-change DCA validation remains outstanding.
3 open issues
What changed in this PR
This PR addresses false positives from issue #7035 by extending the C++ assignment-return query to recognize helpers that return a destination reference parameter.
Changes:
- Adds helper summaries and maps returned parameters to arguments recognized as
*this. - Adds 16 regression cases covering supported helpers and negative controls, with updated expected results.
- Documents the analysis improvement in a change note.
| File | Description |
|---|---|
cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/helpers.cpp |
Adds safe helper cases and negative controls. |
cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 82/AV Rule 82.expected |
Records alerts for the eight new negative controls. |
cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql |
Adds reference-parameter helper recognition. |
cpp/ql/src/change-notes/2026-11-12-assignment-helper-return.md |
Documents the reduction in false positives. |
Review effort: Balanced (auto)
| 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 |
There was a problem hiding this comment.
Addressed in f620085.
The helper summary now rejects any explicit Cast in a reachable return expression's conversion chain before treating it as a direct parameter return. Parentheses remain allowed because ParenthesisExpr is not a Cast.
Added BadHelperReturnCast and BadHelperReturnCStyleCast in casts.cpp: each helper takes a base reference but returns the other same-typed base subobject of a multiply inherited object. Both were missed by the previous commit and are reported after this fix. The existing parenthesized-parameter case and the new parenthesized-argument case remain unreported.
With CodeQL CLI 2.27.4, the expanded AV Rule 82.qlref regression passes, including inline expectations. Separate G++ C++11 runtime checks confirm that these two helpers actually return the other base subobject.
| arg.getUnconverted().getType().getUnspecifiedType() = | ||
| p.getType().getUnspecifiedType().(ReferenceType).getBaseType().getUnspecifiedType() and | ||
| dereferenceThis(arg) |
There was a problem hiding this comment.
Addressed in f620085.
Helper arguments now use a cast-aware mode of the recursive dereferenceThis / pointerThis recognition. It checks conversion chains at each result-producing step, including conditional branches, the right operand of a comma expression, assignment lvalues, member-call qualifiers, and relevant function returns. The pre-existing non-helper cases retain their previous cast handling. An explicitly cast helper-call result is also excluded.
The new tests cover argument/reference casts, pointer casts, conditional and comma paths, a member function returning a cast pointer, a cast member-call qualifier, and a cast helper result. Safe controls retain parentheses and casts in conditions or left comma operands that do not supply the returned reference.
Validation with CLI 2.27.4:
AV Rule 82.qlrefpasses, including inline expectations.- All 12 existing alerts keep the same locations and messages; only the 9 new cast-related alerts are added (12 -> 21). The previous commit misses all 9.
- All 8 existing safe helper operators and 6 new safe operators remain unreported.
- G++ C++11 runtime checks pass for 9 other-base returns and 8 same-object returns.
- Reanalysis of the existing real-GCC reproducer retains the same 3 alerts, with unchanged locations and messages.
- QL formatting and
git diff --checkpass.
These are local results. I have not run the full repository suite or refreshed upstream DCA for this commit; it still needs upstream CI/DCA.
|
Hi @SkyCipherr, Observe that the jsf rules are a very specific set of coding standard rules, see, e.g., https://www.stroustrup.com/JSF-AV-rules.pdf. I'll need to discuss internally, but this might no longer be in the spirit of the AV 82 rule. |
|
@jketema I'm happy with this, assuming DCA doesn't throw up any surprises. 👍 |
|
@SkyCipherr I you able to concisely explain the changes you made without the help of AI? Thanks. |
Summary
Related to #7035.
The
cpp/assignment-does-not-return-thisquery can report an assignment operator that returnshelper(*this, other), even when the helper returns its destination parameter by reference on every reachable return.Add a small reference-parameter summary for helpers with visible, non-virtual definitions. The summary requires a reference parameter and reference return type with matching base types, at least one reachable return, and direct accesses to the same parameter at every reachable return. At the call site, map the returned parameter to its argument, require matching unconverted argument and reference-base types, and use the existing
dereferenceThispredicate to recognize the destination.This handles free and static member helpers, instantiated templates, parameters at different positions, multiple explicit returns, and unreachable alternative returns. It does not infer general alias relationships or follow helper return expressions through casts, local aliases, nested calls, or conditional expressions. Virtual helpers and unavailable definitions remain conservative.
Validation
Validated with CodeQL CLI 2.27.4 against main at
fbf9fca5ce7c3b64c2f5d719ea0444577850cb4a:AV Rule 82.qlrefregression test passes, including inline expectations.git diff --checkpass.The full repository suite, a full build of the original cxon project, and large-project evaluation were not run. Upstream CI has not yet run for this change.