Skip to content

Commit

Permalink
Fix [09a11fb1228f]: Aqua use-after-free can occur when a posted menu …
Browse files Browse the repository at this point in the history
…is destroyed. Thanks to Christopher Chavez.
  • Loading branch information
culler committed Nov 7, 2023
2 parents ad68137 + c204c52 commit 7515e96
Show file tree
Hide file tree
Showing 4 changed files with 124 additions and 5 deletions.
1 change: 1 addition & 0 deletions generic/tkMenu.c
Original file line number Diff line number Diff line change
Expand Up @@ -1179,6 +1179,7 @@ DestroyMenuInstance(
Tcl_EventuallyFree(menuPtr->entries[i], DestroyMenuEntry);
menuPtr->numEntries = i;
}
menuPtr->active = -1;
if (menuPtr->entries != NULL) {
ckfree(menuPtr->entries);
menuPtr->entries = NULL;
Expand Down
13 changes: 8 additions & 5 deletions macosx/tkMacOSXMenu.c
Original file line number Diff line number Diff line change
Expand Up @@ -888,14 +888,17 @@ TkpDestroyMenuEntry(
TKMenu *menu;
NSInteger index;

if (mePtr->platformEntryData && mePtr->menuPtr->platformData) {
menu = (TKMenu *) mePtr->menuPtr->platformData;
if (mePtr->platformEntryData) {
menuItem = (NSMenuItem *) mePtr->platformEntryData;
index = [menu indexOfItem:menuItem];
if (mePtr->menuPtr->platformData) {
menu = (TKMenu *) mePtr->menuPtr->platformData;
index = [menu indexOfItem:menuItem];

if (index > -1) {
[menu removeItemAtIndex:index];
if (index > -1) {
[menu removeItemAtIndex:index];
}
}
[menuItem setTag:(NSInteger) NULL];
[menuItem release];
mePtr->platformEntryData = NULL;
}
Expand Down
81 changes: 81 additions & 0 deletions macosx/tkMacOSXTest.c
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
static Tcl_ObjCmdProc DebuggerObjCmd;
#endif
static Tcl_ObjCmdProc PressButtonObjCmd;
static Tcl_ObjCmdProc MoveMouseObjCmd;
static Tcl_ObjCmdProc InjectKeyEventObjCmd;
static Tcl_ObjCmdProc MenuBarHeightObjCmd;

Expand Down Expand Up @@ -58,6 +59,7 @@ TkplatformtestInit(
Tcl_CreateObjCommand(interp, "debugger", DebuggerObjCmd, NULL, NULL);
#endif
Tcl_CreateObjCommand(interp, "pressbutton", PressButtonObjCmd, NULL, NULL);
Tcl_CreateObjCommand(interp, "movemouse", MoveMouseObjCmd, NULL, NULL);
Tcl_CreateObjCommand(interp, "injectkeyevent", InjectKeyEventObjCmd, NULL, NULL);
Tcl_CreateObjCommand(interp, "menubarheight", MenuBarHeightObjCmd, NULL, NULL);
return TCL_OK;
Expand Down Expand Up @@ -266,6 +268,85 @@ PressButtonObjCmd(
return TCL_OK;
}

/*
*----------------------------------------------------------------------
*
* MoveMouseObjCmd --
*
* This Tcl command simulates a mouse motion to a specific screen
* location. It injects an NSEvent into the NSApplication event queue,
* as opposed to adding events to the Tcl queue as event generate would
* do.
*
* Results:
* A standard Tcl result.
*
* Side effects:
* None.
*
*----------------------------------------------------------------------
*/

static int
MoveMouseObjCmd(
TCL_UNUSED(void *),
Tcl_Interp *interp,
int objc,
Tcl_Obj *const objv[])
{
int x = 0, y = 0, i, value;
CGPoint pt;
NSPoint loc;
NSEvent *motion;
NSArray *screens = [NSScreen screens];
CGFloat ScreenHeight = 0;
enum {X=1, Y};

if (screens && [screens count]) {
ScreenHeight = [[screens objectAtIndex:0] frame].size.height;
}

if (objc != 3) {
Tcl_WrongNumArgs(interp, 1, objv, "x y");
return TCL_ERROR;
}
for (i = 1; i < objc; i++) {
if (Tcl_GetIntFromObj(interp,objv[i],&value) != TCL_OK) {
return TCL_ERROR;
}
switch (i) {
case X:
x = value;
break;
case Y:
y = value;
break;
default:
break;
}
}
pt.x = loc.x = x;
pt.y = y;
loc.y = ScreenHeight - y;

/*
* We set the timestamp to 0 as a signal to tkProcessMouseEvent.
*/

CGWarpMouseCursorPosition(pt);
motion = [NSEvent mouseEventWithType:NSMouseMoved
location:loc
modifierFlags:0
timestamp:0
windowNumber:0
context:nil
eventNumber:0
clickCount:1
pressure:0];
[NSApp postEvent:motion atStart:NO];
return TCL_OK;
}

static int
InjectKeyEventObjCmd(
TCL_UNUSED(void *),
Expand Down
34 changes: 34 additions & 0 deletions tests/menu.test
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ imageInit
# find the earth.gif file for use in these tests (tests 2.*)
set earthPhotoFile [file join [file dirname [info script]] earth.gif]
testConstraint hasEarthPhoto [file exists $earthPhotoFile]
testConstraint pressbutton [llength [info commands pressbutton]]
testConstraint movemouse [llength [info commands movemouse]]

test menu-1.1 {Tk_MenuCmd procedure} -body {
menu
Expand Down Expand Up @@ -4110,6 +4112,38 @@ test menu-39.2 {use-after-free fix - bug 1797555fff} -setup {
destroy .t
} -result {}

test menu-40.1 {Use-after-free if menu destroyed while posted - bug 09a11fb1228f} -setup {
} -constraints {pressbutton} -body {
set done false
event generate {} <Motion> -x 100 -y 100
toplevel .t
menu .t.m
.t.m add command -command {puts Marco} -label Marco
.t.m add command -command {puts Polo} -label Polo
after 1000 {.t.m post 500 500}
after 2000 {destroy .t}
after 2500 {pressbutton 530 510}
after 3000 {set done true}
tkwait variable done
}

test menu-40.2 {Use-after-free if menu destroyed while posted - bug 09a11fb1228f} -setup {
} -constraints {movemouse} -body {
set done false
event generate {} <Motion> -x 100 -y 100
toplevel .t
menu .t.m
.t.m add command -command {puts Marco} -label Marco
.t.m add command -command {puts Polo} -label Polo
after 1000 {.t.m post 500 500}
after 2000 {movemouse 530 510}
after 3000 {destroy .t}
after 3500 {movemouse 530 530}
after 4000 pressbutton 530 530
after 4500 {set done true}
tkwait variable done
pressbutton 530 510
}

# cleanup
imageFinish
Expand Down

0 comments on commit 7515e96

Please sign in to comment.