Add ghostty_surface_set_userdata setter#70
Conversation
Hosts that retain a heap object as the surface userdata need to clear it before freeing that object. A surface can outlive its host-side userdata (e.g. a queued mailbox action drains after the host released its callback context), and surface callbacks would then hand back a dangling pointer. The existing API only exposed a getter. Add a setter so the host can detach (null) the userdata at release time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a new C API function, ChangesSurface Userdata Setter
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR adds
Confidence Score: 5/5Additive, one-line change that mirrors an existing getter; no existing call paths are altered. The change is a single field assignment with a well-documented doc comment, placed directly beside the existing getter, and consistent with the getter's return type. No existing code paths are modified and the new export introduces no side effects. No files require special attention; both changed files are straightforward. Important Files Changed
Sequence DiagramsequenceDiagram
participant Host
participant SurfaceAPI as "Surface (C API)"
participant Mailbox
Host->>SurfaceAPI: "ghostty_surface_new() — sets userdata"
Host->>SurfaceAPI: "ghostty_surface_set_userdata(surface, null)"
Note over Host: "Detaches userdata before freeing callback context"
Host->>Host: "free callback context (safe)"
Mailbox->>SurfaceAPI: "ghostty_surface_userdata()"
SurfaceAPI-->>Mailbox: "null"
Note over Mailbox: "Callback sees null — no dangling dereference"
Reviews (1): Last reviewed commit: "Add ghostty_surface_set_userdata setter" | Re-trigger Greptile |
| const ghostty_surface_config_s*); | ||
| GHOSTTY_API void ghostty_surface_free(ghostty_surface_t); | ||
| GHOSTTY_API void* ghostty_surface_userdata(ghostty_surface_t); | ||
| GHOSTTY_API void ghostty_surface_set_userdata(ghostty_surface_t, void*); |
There was a problem hiding this comment.
The
void* parameter type doesn't communicate to C callers that NULL is a valid and meaningful value (it detaches the userdata). The existing getter already returns plain void* without a nullability annotation, so this is consistent — but it's worth noting for embedder documentation. If the project ever adopts _Nullable/_Nonnull annotations (common in Apple SDKs), this parameter would be the natural first candidate.
| GHOSTTY_API void ghostty_surface_set_userdata(ghostty_surface_t, void*); | |
| GHOSTTY_API void ghostty_surface_set_userdata(ghostty_surface_t, void* _Nullable); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
Adds a
ghostty_surface_set_userdataC API export so embedders can clear (or change) the userdata pointer associated with a surface. The embedded apprt already exposedghostty_surface_userdata(getter) but no setter.Motivation
A host that stores a retained heap object as the surface userdata has no way to detach it before freeing that object. This matters because a surface can outlive the host-side userdata:
Surfaceis still alive.scrollbarfromSurface.updateScrollbar) then drains inApp.drainMailbox→surfaceMessage(which validates the*SurfacewithhasSurface(), so it passes) → the host action callback.ghostty_surface_userdata()and gets the freed pointer back, then dereferences it → use-after-free crash.hasSurface()guards the surface pointer but cannot know the host freed the userdata. With a setter, the host can null the userdata at release time and callbacks safely seenull.This is the upstream half of a fix for an intermittent
EXC_BAD_ACCESSwe hit in cmux (GhosttyApp.handleAction→ freed callback context). The cmux side callsghostty_surface_set_userdata(surface, nil)before releasing its context on the paths where the surface survives.Changes
src/apprt/embedded.zig— newghostty_surface_set_userdataexport next to the existing getter.include/ghostty.h— matching declaration.Risk
Minimal: a single field assignment, additive API, no behavior change for existing callers.
nullis a valid value (detaches userdata).Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a
ghostty_surface_set_userdata(ghostty_surface_t, void*)C API so embedders can clear or change a surface’s userdata. This lets hosts detach freed callback contexts and prevents use‑after‑free when queued callbacks readghostty_surface_userdata()after teardown.Written for commit 9254edc. Summary will update on new commits.
Summary by CodeRabbit
ghostty_surface_set_userdata()C API function for embedded development. This new function enables developers to set and manage custom userdata pointers associated with surface objects. It complements the existing userdata getter functionality, providing bidirectional access to surface-attached data. The function supports null values for detaching custom data associations.