Stop alert handlers reading the action name from freed memory - #142
Open
cortfritz wants to merge 1 commit into
Open
Stop alert handlers reading the action name from freed memory#142cortfritz wants to merge 1 commit into
cortfritz wants to merge 1 commit into
Conversation
Both alert NIFs build their button handlers like this:
NSString *action = btn[@"action"] ?: @"dismiss";
const char *act_c = [action UTF8String];
[ac addAction:[UIAlertAction actionWithTitle:label style:as
handler:^(UIAlertAction *_) {
mob_deliver_alert_action(act_c);
}]];
The block captures a C pointer, so it retains nothing. `action` is owned
by `buttons`, a local the enclosing dispatch block drops on return, and
-UTF8String's buffer is autorelease-scoped on top of that. Both are gone
well before the user taps. mob_deliver_alert_action then hands whatever
occupies that memory to enif_make_atom, so the screen gets an
{:alert, <garbage>} that matches no clause — or a crash, if the bytes are
not valid UTF-8.
Capture the NSString instead and convert inside the handler, where the
pointer only has to survive the enif_make_atom call.
This is hard to see in testing: freed bytes usually still spell the old
string, so it works until the allocator reuses them. AGENTS.md gets it as
pre-empt rule 15.
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 freeto 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.
Both alert NIFs build their button handlers from a dangling pointer:
The block captures a
const char *, so it retains nothing.actionbelongs tobuttons, a local the enclosingdispatch_asyncblock drops as soon as it returns — and-UTF8String's buffer is autorelease-scoped besides. Both are gone long before the user taps.mob_deliver_alert_actionthen passes that memory toenif_make_atom, so the screen receives an{:alert, <whatever was there>}matching no clause, or the NIF call fails outright if the bytes are not valid UTF-8.Change
Capture the
NSString— ARC retains it for the life of the handler — and convert inside the block, where the pointer only has to survive theenif_make_atomcall. Applied tonif_alert_showandnif_action_sheet_show.This is the kind of thing that hides in testing, because freed bytes usually still spell the old string until the allocator reuses them, so it is added to
AGENTS.mdas pre-empt rule 15.Verification
Rung 1.
xcrun clang-format --dry-run -Werror ios/mob_nif.m— clean.clang -fsyntax-onlyagainst the iOS 26.5 SDK — no diagnostics introduced.Independent of the companion PR as a diff — different hunks, no conflict — but only observable once alerts present.
https://claude.ai/code/session_01FGHFA67yQndR78WYAU9DM7