fix(runtime): free struct buffers marshalled from object literals when the call completes - #449
Draft
edusperoni wants to merge 1 commit into
Draft
fix(runtime): free struct buffers marshalled from object literals when the call completes#449edusperoni wants to merge 1 commit into
edusperoni wants to merge 1 commit into
Conversation
…n the call completes A plain JS object passed where native expects a struct pointer (MyStruct*) is snapshotted into a malloc'd buffer that was never freed - the long- standing 'How to free this?' TODO in Interop::WriteValue, and the last remaining entry from the Instruments leaks run addressed by 8080bc0. The buffer is now owned by the FFICall driving the invocation and freed once the call completes, making the literal form a call-scoped borrow. Only SetFFIParams passes the owner: writes into interop.Reference slots (where the pointer outlives the call) and nested ref/out-param initialization deliberately keep the unowned allocation. BREAKING-ish: native APIs that retained such a pointer past the call only worked because of the leak; they must now be fed a wrapped struct instance (caller-kept) or interop.alloc memory (manual/callee-freed) instead.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes
Passing a plain JS object literal where a native parameter expects a pointer to a struct (e.g.
CGPoint*,NSRange*):previously snapshotted the literal into a
malloc'd buffer that was never freed (the long-standing// TODO: How to free this?inInterop::WriteValue) — one leak per call. With this PR the buffer is owned by theFFICalldriving the invocation and freed when the call completes (afterffi_callreturns and the result is read).Writes of literals into
MyStruct*-typed slots of aninterop.Referenceare unchanged: that pointer is stored in memory that outlives any call, so those buffers deliberately remain unowned.The lifetime contract (new, documented behavior)
{x, y}CGPointMake(...),new TNSSimpleStruct(...))interop.alloc(...)(+interop.Reference)interop.freeor never)This is a deliberate behavior change with a known hazard class: code that passes a literal and relies on native retaining that pointer past the call worked before only because of the leak (the buffer was accidentally immortal). Under this PR such code gets a dangling pointer at call end — a use-after-free instead of a leak. Symmetrically, an API whose contract is "callee frees the pointer" would now double-free;
interop.allocis the correct form there.Mitigating data — a survey of NativeScript core (
packages/core+ test apps):sockaddr→SCNetworkReachabilityCreateWithAddress,connectivity/index.ios.ts) already usesnew interop.Reference(sockaddr, {...})— a different, unaffected path.interop.Referenceuses are scalar out-params (CGFloat*,BOOL*,NSError**).So exposure is limited to third-party plugins / app code using a pattern core never uses. Still: this should soak in real apps (ideally ones heavy on CoreGraphics/CoreText/AV struct-pointer APIs) before it ships, and the release notes should state the new lifetime rule.
Implementation
FFICallgainsOwnBuffer(void*)+ an owned-buffer list freed in its destructor (stack-scoped at all three drive sites:CallInitializer,CallFunctionInternal, block invoke — verified the destructor runs after the result is read).Interop::WriteValuetakes an optionalFFICall* callOwner = nullptr; onlySetFFIParamspasses it. All nine other call sites — including nested ref/out-param initialization andReference.cppslot writes — stay owner-less on purpose (audited).+[TNSTestNativeCallbacks recordsPointerEcho:](returns the pointee) to allow per-iteration value assertions.Tests
New specs in
Marshalling/RecordTests.js:interop.Reference→ pointer param: reference stays readable after the call;Full suite passes. The leak itself is verified fixed out-of-band with the Instruments Leaks template — this path was the last remaining entry from the leaks run that 8080bc0 addressed.