Reclaim owned right operand of string concat (leak fix)#24
Conversation
An owned string temporary used as the right operand of `+` (e.g. `"row-" + int_to_string(n)`) was copied by mako_str_concat but never freed — one buffer leaked per concat, accumulating in a long-running builder. The right operand is now freed after the concat when it is an unambiguously owned temporary (gated by expr_is_scope_drop_safe, which excludes literal views / index / method borrows, so no double-free). int_to_string and format_int results are recognized as owned. Verified on Linux: concat churn leak-free under LeakSanitizer, full suite green under AddressSanitizer (no double-free/UAF). New concat_owned_operand_test.mko.
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughString concatenation code generation now detects safe owned right-hand operands, frees their temporary buffers after concatenation, and recognizes integer-string helpers in leak handling. New example tests cover direct, helper-returned, chained, and value-correct concatenations. ChangesString concatenation ownership
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CodeGenerator
participant GeneratedConcat
participant RightOperandBuffer
CodeGenerator->>GeneratedConcat: Emit ownership-aware concat code
GeneratedConcat->>RightOperandBuffer: Bind owned right operand
GeneratedConcat->>RightOperandBuffer: Free buffer after mako_str_concat
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/testing/concat_owned_operand_test.mko (1)
25-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the stress-test value assertions.
TestConcatReturnedBuilderonly checks that the aggregate length is positive, so an incorrect non-emptylabelresult can pass.TestConcatChainshould use exact equality instead of substring containment to catch extra output.Also applies to: 35-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/testing/concat_owned_operand_test.mko` around lines 25 - 30, Strengthen assertions in TestConcatReturnedBuilder and TestConcatChain: replace the aggregate length positivity check with equality against the exact expected total for labels 0 through 63, and replace substring containment in TestConcatChain with exact string equality against the expected concatenated result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@examples/testing/concat_owned_operand_test.mko`:
- Around line 25-30: Strengthen assertions in TestConcatReturnedBuilder and
TestConcatChain: replace the aggregate length positivity check with equality
against the exact expected total for labels 0 through 63, and replace substring
containment in TestConcatChain with exact string equality against the expected
concatenated result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eba9f0d2-527c-4171-b487-84b0ffb565b1
📒 Files selected for processing (3)
CHANGELOG.mdexamples/testing/concat_owned_operand_test.mkosrc/codegen/mod.rs
Recognize base64_encode / base32_encode / base64_decode / bytes_to_hex / csv_join_row / auth_bearer / auth_basic_header as owned producers so their results free at scope exit / after concat instead of leaking one buffer per call. Their output is freshly computed (never a borrowed view of an argument), so this is double-free-safe. string(x) stays excluded — it is polymorphic (identity for string input). Verified on Linux: base64_encode drops from 2 to 1 leaked allocation per call (result reclaimed; the residual is an internal builtin-impl leak, tracked separately); full suite green under AddressSanitizer (380/0, no double-free/UAF).
|
Superseded by the rebase onto the networking/Diameter main; the fixes are re-applied and re-validated in the new PR. |
2 participants
Fixes a pre-existing per-call leak: an owned string temporary used as the right operand of
+(e.g."row-" + int_to_string(n)) was copied by mako_str_concat but never freed (mako_str_concat_own only consumes the left operand). One buffer leaked per concat, accumulating in long-running string builders.The right operand is now freed after the concat when it is an unambiguously owned temporary, gated by expr_is_scope_drop_safe (excludes literal views, index, and method borrows — no double-free). int_to_string / format_int recognized as owned.
Verification (Linux): concat churn leak-free under LeakSanitizer (was 499 leaks); full suite green under AddressSanitizer (380/0, no double-free/UAF); both backends pass new concat_owned_operand_test.mko. Follows #23 (same leak family).
Summary by CodeRabbit