fix(v3/darwin): copy screen id/name out of autoreleased UTF8String buffers by leaanthony · Pull Request #5565 · wailsapp/wails

leaanthony · 2026-06-10T21:23:55Z

Description

processScreen stored the raw result of -[NSString UTF8String] in the C Screen struct's id and name fields. UTF8String returns a pointer into an autoreleased buffer whose lifetime ends when the enclosing autorelease pool drains — which can happen before Go reads the strings with C.GoString in cScreenToScreen. The result is a use-after-free that intermittently aborts the process with fatal error: invalid pointer found on stack when Wails' internal ApplicationDidChangeScreenParameters handler refreshes the screen cache (display sleep/wake, monitor connect/disconnect, resolution change). Because it's a runtime memory-safety abort inside an internal handler, applications cannot recover() from it.

Changes

id and name are now strdup'd into malloc'd memory owned by the struct, and freed exactly once by cScreenToScreen after conversion.
name is initialised to NULL, fixing a second latent bug: on macOS < 10.15 the field was left uninitialised (garbage pointer fed to GoString).
getAllScreens / getScreenForWindow now run inside an explicit @autoreleasepool. Previously, calls from Go goroutine threads (which have no ambient pool) leaked every autoreleased object created during enumeration — and this is the common path, since application event handlers run on goroutines.
Extracted allScreens() so the regression test can exercise the ownership handover (cgo isn't available in test files).

Test (proves the issue)

TestScreenStringsSurviveAutoreleasePool enumerates screens — with the explicit pool, the autoreleased buffers are deterministically dead by the time Go reads them, so the test reproduces the report's timing-dependent crash reliably:

Without the strdup fix (run with MallocScribble=1): segfaults in the string read — the exact crash signature from macOS: fatal error: invalid pointer found on stack in cScreenToScreen on display change (UTF8String use-after-free) #5556.
With the fix: passes, real screen data verified on a 2-display setup (ID="1" Name="Built-in Retina Display", ID="5" Name="LS49A950U").

Full pkg/application suite passes on macOS (Apple Silicon).

Relationship to #5516

#5516 addresses the concurrent off-main-thread enumeration crash and the screen-count TOCTOU. This PR is orthogonal: it fixes the dangling string pointers (and the pre-10.15 uninitialised name), which #5516 does not touch. The reporter's runtime.gostring(0x3) stack is this bug. Both can land independently; whichever lands second has a trivial conflict in processScreen/processAndCacheScreens.

Fixes #5556

Type of change

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code follows the general coding style of this project
I have added tests that prove my fix is effective
I have updated v3/UNRELEASED_CHANGELOG.md

Summary by CodeRabbit

Bug Fixes
- Fixed a macOS crash during screen enumeration after display changes; eliminated use-after-free and avoided leaks when reading screen identifiers and names across language/runtime boundaries.
Tests
- Added a macOS-only test to verify screen IDs and names remain valid after display changes and resource cleanup.

…ffers processScreen stored the raw result of -[NSString UTF8String] in the C Screen struct's id and name fields. UTF8String returns a pointer into an autoreleased buffer whose lifetime ends when the enclosing autorelease pool drains, which can happen before Go reads the strings with C.GoString in cScreenToScreen — a use-after-free that intermittently aborts the process with 'fatal error: invalid pointer found on stack' when Wails' internal ApplicationDidChangeScreenParameters handler refreshes the screen cache (display sleep/wake, monitor connect or disconnect, resolution change). Because it is a runtime memory-safety abort inside an internal handler, applications cannot recover from it. The strings are now strdup'd into malloc'd memory owned by the struct and freed by cScreenToScreen after conversion. name is also initialised to NULL so the pre-10.15 path no longer leaves an uninitialised pointer. getAllScreens and getScreenForWindow now run inside an explicit autorelease pool: previously, calls from Go goroutine threads (which have no ambient pool) leaked every autoreleased object created during enumeration. The pool also makes the bug deterministic instead of timing-dependent, which the new regression test exploits — run with MallocScribble=1 it segfaults without the fix and passes with it. Fixes #5556

coderabbitai · 2026-06-10T21:24:10Z

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d0c13baf-14f7-4e08-80a6-673142674940

📥 Commits

Reviewing files that changed from the base of the PR and between 826a787 and bf467f4.

📒 Files selected for processing (1)

v3/UNRELEASED_CHANGELOG.md

Walkthrough

Replaces autoreleased NSString UTF8String pointers with strdup'd C strings, wraps Objective‑C enumeration in autorelease pools, converts and frees C buffers in Go, adds allScreens() helper, a Darwin regression test, and a changelog entry.

Changes

macOS Screen String Ownership Fix

Layer / File(s)	Summary
C includes and strdup helper `v3/pkg/application/screen_darwin.go`	Added `string.h` and `strdupOrNull` to safely duplicate C strings for cross-language ownership.
Populate screen id/name with strdup'd C strings `v3/pkg/application/screen_darwin.go`	Store screen `id` (and `name` on macOS 10.15+) as `strdupOrNull`-allocated `char*` instead of pointers to autoreleased `NSString` UTF8 buffers.
Enumeration APIs and autoreleasepool wrapping `v3/pkg/application/screen_darwin.go`	`getAllScreens` now returns an allocated array with an out-count; `getAllScreens` and `getScreenForWindow` run inside `@autoreleasepool` blocks to constrain autorelease lifetimes.
Go conversion, freeing, and helper function `v3/pkg/application/screen_darwin.go`	`cScreenToScreen` converts C `id`/`name` to Go strings then `C.free`s the malloc'd pointers. Added `allScreens()` to call the new C API and free the C array; `processAndCacheScreens` now uses `LayoutScreens(allScreens())`.
Regression test and changelog `v3/pkg/application/screen_darwin_test.go`, `v3/UNRELEASED_CHANGELOG.md`	Added `TestScreenStringsSurviveAutoreleasePool` (Darwin-only) to validate ID parsing and Name UTF‑8 validity, skipping when headless; documented fix in changelog.

Sequence Diagram

sequenceDiagram
  participant ObjC as Objective-C
  participant C as C layer
  participant Go as Go runtime
  ObjC->>C: create NSStrings for screen id/name
  C->>C: strdupOrNull(UTF8String) -> malloc'd char*
  C->>Go: return array of C Screens (malloc'd) + count
  Go->>Go: allScreens()/cScreenToScreen iterate
  Go->>C: C.GoString(char*) to make Go string
  Go->>C: C.free(char*) to release malloc'd memory

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

wailsapp/wails#5168: Modifies v3/pkg/application/screen_darwin.go's screen handling (addresses Retina scaling), related by file overlap.

Suggested labels

Bug, MacOS, v3-alpha, size:M, awaiting review 🕝

Suggested reviewers

atterpac

Poem

🐰 I hopped through pools where pointers played,
Freed bytes danced, then ran away;
I stitched them safe with strdup's art,
Freed them later — careful part.
Now screens sleep sound, no crashful day.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and concisely describes the main fix: copying screen id/name out of autoreleased UTF8String buffers, which is the core issue being addressed.
Description check	✅ Passed	The PR description comprehensively covers the root cause, changes made, testing approach, and relationship to related issues. It follows the template structure with issue link, type of change, and checklist items completed.
Linked Issues check	✅ Passed	The PR directly addresses all coding objectives from `#5556`: eliminates the use-after-free by strdup'ing UTF8String results, initializes the name field, wraps enumeration in explicit autoreleasepool, and provides a deterministic regression test with real screen data validation.
Out of Scope Changes check	✅ Passed	All changes are within scope: screen_darwin.go implementation fixes, screen_darwin_test.go adds the required regression test, and UNRELEASED_CHANGELOG.md documents the fix as required.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/screen-darwin-uaf-5556

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull request overview

Fixes a macOS memory-safety issue in v3 screen enumeration where -[NSString UTF8String] pointers were stored into C structs and later read by Go after the autorelease pool drained, causing intermittent use-after-free crashes during display-change events.

Changes:

Copy screen id/name into malloc-owned buffers (strdup) and free them after C.GoString conversion.
Wrap getAllScreens / getScreenForWindow in explicit @autoreleasepool blocks to avoid leaking autoreleased objects when called from Go goroutine threads.
Add a Darwin regression test and update the unreleased changelog.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
v3/UNRELEASED_CHANGELOG.md	Documents the macOS screen-string use-after-free crash fix and autorelease pool changes.
v3/pkg/application/screen_darwin.go	Implements strdup-based ownership for screen strings, adds autorelease pools, and adjusts screen enumeration/conversion flow.
v3/pkg/application/screen_darwin_test.go	Adds a regression test intended to catch invalid screen string lifetimes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

leaanthony · 2026-06-13T03:13:44Z

+		NSArray<NSScreen *> *screens = [NSScreen screens];
+		Screen* returnScreens = malloc(sizeof(Screen) * screens.count);
+		for (int i = 0; i < screens.count; i++) {


Resolved in commit 826a787c6. getAllScreens now takes an int* outCount and fills it from the same [NSScreen screens] snapshot used to size the malloc, and allScreens() reads that count instead of calling GetNumScreens() separately — so allocation and iteration use one consistent snapshot, closing the TOCTOU/over-read (and the tail-entry strdup leak on shrink). Verified with the MallocScribble=1 regression test plus the full pkg/application suite on macOS.

coderabbitai

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@v3/pkg/application/screen_darwin.go`:
- Around line 110-123: getAllScreens() currently returns only a pointer while
allScreens() calls C.GetNumScreens() separately, creating a TOCTOU risk; change
getAllScreens() to return both the allocated Screen* and its count (e.g., a
small struct or out-param) so the same NSScreen snapshot/alloc is used, then
update allScreens() to use the returned count instead of calling
C.GetNumScreens(); ensure cScreens is sized from that returned count, pass that
same count into cScreenToScreen conversions, and keep the `@autoreleasepool`
around the NSScreen enumeration to avoid leaks from processScreen()/strdup'd
id/name.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 603f78bb-66bd-4572-92ef-6422c098d06e

📥 Commits

Reviewing files that changed from the base of the PR and between 43309dd and 7d90f3d.

📒 Files selected for processing (3)

v3/UNRELEASED_CHANGELOG.md
v3/pkg/application/screen_darwin.go
v3/pkg/application/screen_darwin_test.go

Review follow-up: allScreens() sized its slice from a separate GetNumScreens() call. If the display configuration changed between the two calls, Go could read past the malloc'd array (freeing garbage id/name pointers) or, when shrinking, leak the strdup'd strings in unvisited tail entries. getAllScreens now returns the count from the same [NSScreen screens] snapshot via an out-parameter.

leaanthony · 2026-06-10T22:38:45Z

Addressed in the latest commit: getAllScreens now returns the count via an out-parameter from the same [NSScreen screens] snapshot used to size the allocation, and allScreens() no longer calls GetNumScreens(). This closes the over-read/leak window within this PR rather than deferring it to #5516 (whichever lands second has a small conflict to resolve either way). MallocScribble=1 regression test and the full pkg/application suite still pass on macOS.

Copilot AI review requested due to automatic review settings June 10, 2026 21:23

Copilot started reviewing on behalf of leaanthony June 10, 2026 21:24 View session

Copilot AI reviewed Jun 10, 2026

View reviewed changes

coderabbitai Bot reviewed Jun 10, 2026

View reviewed changes

Comment thread v3/pkg/application/screen_darwin.go Outdated

leaanthony mentioned this pull request Jun 10, 2026

[v3, macOS] Screen.Bounds.X/Y on non-primary screens are off by the screen's scale factor (mixed-DPI) #5409

Open

Merge branch 'master' into fix/screen-darwin-uaf-5556

bf467f4

leaanthony merged commit b553ca4 into master Jun 13, 2026
12 of 16 checks passed

leaanthony deleted the fix/screen-darwin-uaf-5556 branch June 13, 2026 03:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(v3/darwin): copy screen id/name out of autoreleased UTF8String buffers#5565

fix(v3/darwin): copy screen id/name out of autoreleased UTF8String buffers#5565
leaanthony merged 3 commits into
masterfrom
fix/screen-darwin-uaf-5556

leaanthony commented Jun 10, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 10, 2026 •

edited

Loading

Review failed

❌ Failed checks (1 warning)

Uh oh!

Copilot AI left a comment

Uh oh!

leaanthony Jun 13, 2026

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

leaanthony commented Jun 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

leaanthony commented Jun 10, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Test (proves the issue)

Relationship to #5516

Type of change

Checklist:

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Sequence Diagram

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

leaanthony Jun 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leaanthony commented Jun 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

leaanthony commented Jun 10, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 10, 2026 •

edited

Loading