font: guard null CTFontCopyDisplayName in DeferredFace.name - #94
font: guard null CTFontCopyDisplayName in DeferredFace.name#94lawrencecchen wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughFont.copyDisplayName's return type was changed from a non-null pointer to a nullable pointer (?*foundation.String). DeferredFace.name was updated to handle the null case by logging a warning with font pointer and variation count, then returning an empty string. ChangesCoreText Display Name Nullability
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
🚥 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 |
Greptile SummaryFixes a crash (
Confidence Score: 4/5Safe to merge; the crash fix is correct and the change is narrowly scoped to the null case. The null-guard and optional return type are correct and consistent with the established copyAttribute pattern in the same file. The only outstanding concern is a pre-existing CFString memory leak in DeferredFace.name — display_name is never released after CTFontCopyDisplayName, and the same is true of copyFamilyName in face/coretext.zig. This PR does not introduce the leak but touches the affected path without addressing it. src/font/DeferredFace.zig — the CoreText branch of name() leaks the CFString returned by copyDisplayName on every call. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller
participant DeferredFace
participant Font as pkg/macos/text/Font
participant CT as CoreText C API
Caller->>DeferredFace: name(buf)
DeferredFace->>Font: copyDisplayName()
Font->>CT: CTFontCopyDisplayName(font_ref)
CT-->>Font: CFStringRef (possibly NULL)
Note over Font: @ptrFromInt(@intFromPtr(...))<br/>maps NULL to Zig null
Font-->>DeferredFace: "?*foundation.String"
alt display_name is null (new guard)
DeferredFace->>DeferredFace: log.warn(...)
DeferredFace-->>Caller: return empty string
else display_name is non-null
DeferredFace->>Font: cstringPtr(.utf8)
alt fast path (inline storage)
Font-->>DeferredFace: "*const u8 pointer"
DeferredFace-->>Caller: []const u8 slice
else slow path (allocation needed)
DeferredFace->>Font: cstring(buf, .utf8)
Font-->>DeferredFace: copied into buf
DeferredFace-->>Caller: buf slice
end
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Caller
participant DeferredFace
participant Font as pkg/macos/text/Font
participant CT as CoreText C API
Caller->>DeferredFace: name(buf)
DeferredFace->>Font: copyDisplayName()
Font->>CT: CTFontCopyDisplayName(font_ref)
CT-->>Font: CFStringRef (possibly NULL)
Note over Font: @ptrFromInt(@intFromPtr(...))<br/>maps NULL to Zig null
Font-->>DeferredFace: "?*foundation.String"
alt display_name is null (new guard)
DeferredFace->>DeferredFace: log.warn(...)
DeferredFace-->>Caller: return empty string
else display_name is non-null
DeferredFace->>Font: cstringPtr(.utf8)
alt fast path (inline storage)
Font-->>DeferredFace: "*const u8 pointer"
DeferredFace-->>Caller: []const u8 slice
else slow path (allocation needed)
DeferredFace->>Font: cstring(buf, .utf8)
Font-->>DeferredFace: copied into buf
DeferredFace-->>Caller: buf slice
end
end
|
CTFontCopyDisplayName can return NULL; the @ptrFromInt(@intFromPtr(...)) laundering turned that into a non-optional pointer at address 0, and the subsequent CFStringGetCStringPtr deref crashed (EXC_BAD_ACCESS, Sentry CMUXTERM-MACOS-1C0Z, manaflow-ai/cmux#7312). Make copyDisplayName return an optional and fall back to an empty name with a warning log in DeferredFace.name.
411b866 to
64c9c97
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/macos/text/font.zig (1)
155-157: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSibling functions have the same latent null-crash pattern fixed here for
copyDisplayName.
copyFamilyNameandcopyPostScriptNameuse the identical@ptrFromInt(@intFromPtr(...))unpacking but declare non-optional return types.CTFontCopyFamilyName/CTFontCopyPostScriptNamecan also returnNULLfor malformed/corrupt fonts, per real-world reports (e.g. WebKit bug 237373: "crashes when given a font with a nil postscript name, font family, or display name"). These would suffer the exact sameEXC_BAD_ACCESSthis PR fixes for display name.Consider making these two nullable as well, for consistency and to close the same class of bug preemptively — though this is out of the stated scope of this PR (display name only).
♻️ Suggested follow-up (separate from this PR's scope)
- pub fn copyFamilyName(self: *Font) *foundation.String { + pub fn copyFamilyName(self: *Font) ?*foundation.String { return `@ptrFromInt`(`@intFromPtr`(c.CTFontCopyFamilyName(`@ptrCast`(self)))); } ... - pub fn copyPostScriptName(self: *Font) *foundation.String { + pub fn copyPostScriptName(self: *Font) ?*foundation.String { return `@ptrFromInt`(`@intFromPtr`(c.CTFontCopyPostScriptName(`@ptrCast`(self)))); }Also applies to: 163-165
🤖 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 `@pkg/macos/text/font.zig` around lines 155 - 157, `copyFamilyName` and `copyPostScriptName` use the same unsafe pointer-unwrapping pattern as `copyDisplayName` and can also crash if CoreText returns NULL for malformed fonts. Update these sibling methods in `Font` to use nullable return types and handle a null result safely, matching the fix applied to `copyDisplayName` so the same EXC_BAD_ACCESS path is closed consistently.
🤖 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 `@pkg/macos/text/font.zig`:
- Around line 155-157: `copyFamilyName` and `copyPostScriptName` use the same
unsafe pointer-unwrapping pattern as `copyDisplayName` and can also crash if
CoreText returns NULL for malformed fonts. Update these sibling methods in
`Font` to use nullable return types and handle a null result safely, matching
the fix applied to `copyDisplayName` so the same EXC_BAD_ACCESS path is closed
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6600aa66-be7d-43f8-8637-2ec635653f84
📒 Files selected for processing (2)
pkg/macos/text/font.zigsrc/font/DeferredFace.zig
1 participant
CTFontCopyDisplayName can return NULL; the
@ptrFromInt(@intFromPtr(...))laundering turned that into a non-optional pointer at address 0, and the subsequent CFStringGetCStringPtr deref crashed with EXC_BAD_ACCESS (Sentry CMUXTERM-MACOS-1C0Z, manaflow-ai/cmux#7312).copyDisplayNamenow returns an optional andDeferredFace.namefalls back to an empty name with a warning log. The only non-test caller is the CoreText branch ofDeferredFace.name.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes a macOS crash in the CoreText path when CTFontCopyDisplayName returns NULL. copyDisplayName now returns an optional; DeferredFace.name logs a warning and falls back to an empty string to prevent EXC_BAD_ACCESS.
Written for commit 64c9c97. Summary will update on new commits.
Summary by CodeRabbit