Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[iOSTextInput] fix potential dangling pointer access #26547

Merged
fluttergithubbot merged 7 commits into from
Jun 4, 2021
Merged

[iOSTextInput] fix potential dangling pointer access #26547

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d899b03
[iOSTextInput] fix potential dangling pointer access
LongCatIsLooong Jun 2, 2021
1a0e8ab
code comments
LongCatIsLooong Jun 2, 2021
69b5f46
[iOSTextInput] fix potential dangling pointer access
LongCatIsLooong Jun 2, 2021
1633a8a
[iOSTextInput] fix potential dangling pointer access
LongCatIsLooong Jun 2, 2021
95df7bf
Merge remote-tracking branch 'upstream/master' into dont-reuse-textin…
LongCatIsLooong Jun 3, 2021
a5786e0
review
LongCatIsLooong Jun 4, 2021
456c7e1
fix tests
LongCatIsLooong Jun 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 76 additions & 40 deletions shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,9 @@ @implementation FlutterTextInputView {
const char* _selectionAffinity;
FlutterTextRange* _selectedTextRange;
CGRect _cachedFirstRect;
// The view has reached end of life, and is no longer
// allowed to access its textInputDelegate.
BOOL _decommissioned;
Copy link
Member

@gaaclarke gaaclarke Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this extra state, wouldn't removal of the FlutterTextInputView be enough to say it's decommissioned?

Copy link
Member

@gaaclarke gaaclarke Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the crux of your problem was the reuse of the views, right? Which you removed elsewhere.

Copy link
Contributor Author

@LongCatIsLooong LongCatIsLooong Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crux of the problem is UIKit actually holds onto "decommissioned" views (e.g. UITextInteraction may keep a view alive after it's removed from its superview) and attempts to change the selection in the view (as a result the view will try to access the engine and send the update to the framework).

Copy link
Member

@gaaclarke gaaclarke Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, do we know who is holding onto the view? It might just be a matter of clearing out it's state as well. _textInterfaction.view = nil?

Copy link
Contributor Author

@LongCatIsLooong LongCatIsLooong Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Sorry had to context switch). I think I tried that but didn't work. UITextInteraction.textInput is a weak ref: https://developer.apple.com/documentation/uikit/uitextinteraction/3255084-textinput?language=objc, UIKit seems to be retaining it somewhere else and removing the weak ref doesn't seem to do anything other than setting it to nil.

Copy link
Contributor Author

@LongCatIsLooong LongCatIsLooong Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure there's a public API that allows us to release the reference. I think technically it's reasonable for uikit to retain a strong ref to a UITextInput instance until it's not needed.

Copy link
Contributor Author

@LongCatIsLooong LongCatIsLooong Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regarding setting _textInputDelegate instead (the first comment), I thought about that but slightly prefer having an additional lifecycle tracking variable that's irreversible. Then I'll be able to add asserts to verify that decommissioned views are not gonna get reused. But no strong preference.

Copy link
Member

@gaaclarke gaaclarke Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it's kind of weird but sounds like you've looking into all the alternatives so I can go along with this. Thanks for looking into it.

}

@synthesize tokenizer = _tokenizer;
Expand Down Expand Up @@ -535,6 +538,7 @@ - (instancetype)init {
_returnKeyType = UIReturnKeyDone;
_secureTextEntry = NO;
_accessibilityEnabled = NO;
_decommissioned = NO;
if (@available(iOS 11.0, *)) {
_smartQuotesType = UITextSmartQuotesTypeYes;
_smartDashesType = UITextSmartDashesTypeYes;
Expand All @@ -545,6 +549,7 @@ - (instancetype)init {
}

- (void)configureWithDictionary:(NSDictionary*)configuration {
NSAssert(!_decommissioned, @"Attempt to reuse a decommissioned view, for %@", configuration);
NSDictionary* inputType = configuration[kKeyboardType];
NSString* keyboardAppearance = configuration[kKeyboardAppearance];
NSDictionary* autofill = configuration[kAutofillProperties];
Expand Down Expand Up @@ -596,6 +601,23 @@ - (UITextContentType)textContentType {
return _textContentType;
}

- (id<FlutterTextInputDelegate>)textInputDelegate {
return _decommissioned ? nil : _textInputDelegate;
}

// Declares that the view has reached end of life, and
// is no longer allowed to access its textInputDelegate.
//
// UIKit may retain this view (even after it's been removed
// from the view hierarchy) so that it may outlive the plugin/engine,
// in which case _textInputDelegate will become a dangling pointer.

// The text input plugin needs to call decommision when it should
// not have access to its FlutterTextInputDelegate any more.
- (void)decommision {
_decommissioned = YES;
}

- (void)dealloc {
[_text release];
[_markedText release];
Expand Down Expand Up @@ -778,7 +800,8 @@ - (void)replaceRange:(UITextRange*)range withText:(NSString*)text {

- (BOOL)shouldChangeTextInRange:(UITextRange*)range replacementText:(NSString*)text {
if (self.returnKeyType == UIReturnKeyDefault && [text isEqualToString:@"\n"]) {
[_textInputDelegate performAction:FlutterTextInputActionNewline withClient:_textInputClient];
[self.textInputDelegate performAction:FlutterTextInputActionNewline
withClient:_textInputClient];
return YES;
}

Expand Down Expand Up @@ -819,7 +842,7 @@ - (BOOL)shouldChangeTextInRange:(UITextRange*)range replacementText:(NSString*)t
break;
}

[_textInputDelegate performAction:action withClient:_textInputClient];
[self.textInputDelegate performAction:action withClient:_textInputClient];
return NO;
}

Expand Down Expand Up @@ -1062,9 +1085,9 @@ - (CGRect)firstRectForRange:(UITextRange*)range {
return _cachedFirstRect;
}

[_textInputDelegate showAutocorrectionPromptRectForStart:start
end:end
withClient:_textInputClient];
[self.textInputDelegate showAutocorrectionPromptRectForStart:start
end:end
withClient:_textInputClient];
// TODO(cbracken) Implement.
return CGRectZero;
}
Expand Down Expand Up @@ -1097,21 +1120,21 @@ - (UITextRange*)characterRangeAtPoint:(CGPoint)point {
}

- (void)beginFloatingCursorAtPoint:(CGPoint)point {
[_textInputDelegate updateFloatingCursor:FlutterFloatingCursorDragStateStart
withClient:_textInputClient
withPosition:@{@"X" : @(point.x), @"Y" : @(point.y)}];
[self.textInputDelegate updateFloatingCursor:FlutterFloatingCursorDragStateStart
withClient:_textInputClient
withPosition:@{@"X" : @(point.x), @"Y" : @(point.y)}];
}

- (void)updateFloatingCursorAtPoint:(CGPoint)point {
[_textInputDelegate updateFloatingCursor:FlutterFloatingCursorDragStateUpdate
withClient:_textInputClient
withPosition:@{@"X" : @(point.x), @"Y" : @(point.y)}];
[self.textInputDelegate updateFloatingCursor:FlutterFloatingCursorDragStateUpdate
withClient:_textInputClient
withPosition:@{@"X" : @(point.x), @"Y" : @(point.y)}];
}

- (void)endFloatingCursor {
[_textInputDelegate updateFloatingCursor:FlutterFloatingCursorDragStateEnd
withClient:_textInputClient
withPosition:@{@"X" : @(0), @"Y" : @(0)}];
[self.textInputDelegate updateFloatingCursor:FlutterFloatingCursorDragStateEnd
withClient:_textInputClient
withPosition:@{@"X" : @(0), @"Y" : @(0)}];
}

#pragma mark - UIKeyInput Overrides
Expand Down Expand Up @@ -1139,9 +1162,11 @@ - (void)updateEditingState {
};

if (_textInputClient == 0 && _autofillId != nil) {
[_textInputDelegate updateEditingClient:_textInputClient withState:state withTag:_autofillId];
[self.textInputDelegate updateEditingClient:_textInputClient
withState:state
withTag:_autofillId];
} else {
[_textInputDelegate updateEditingClient:_textInputClient withState:state];
[self.textInputDelegate updateEditingClient:_textInputClient withState:state];
}
}

Expand Down Expand Up @@ -1259,8 +1284,6 @@ - (void)enableActiveViewAccessibility {
@end

@interface FlutterTextInputPlugin ()
@property(nonatomic, strong) FlutterTextInputView* reusableInputView;

// The current password-autofillable input fields that have yet to be saved.
@property(nonatomic, readonly)
NSMutableDictionary<NSString*, FlutterTextInputView*>* autofillContext;
Expand All @@ -1278,11 +1301,11 @@ - (instancetype)init {
self = [super init];

if (self) {
_reusableInputView = [[FlutterTextInputView alloc] init];
_reusableInputView.secureTextEntry = NO;
_autofillContext = [[NSMutableDictionary alloc] init];
_activeView = [_reusableInputView retain];
_inputHider = [[FlutterTextInputViewAccessibilityHider alloc] init];
// Initialize activeView with a dummy view to keep tests
// passing.
_activeView = [[FlutterTextInputView alloc] init];
}

return self;
Expand All @@ -1291,7 +1314,6 @@ - (instancetype)init {
- (void)dealloc {
[self hideTextInput];
_activeView.textInputDelegate = nil;
[_reusableInputView release];
[_activeView release];
[_inputHider release];
[_autofillContext release];
Expand Down Expand Up @@ -1398,14 +1420,14 @@ - (void)triggerAutofillSave:(BOOL)saveEntries {
if (saveEntries) {
// Make all the input fields in the autofill context visible,
// then remove them to trigger autofill save.
[self cleanUpViewHierarchy:YES clearText:YES];
[self cleanUpViewHierarchy:YES clearText:YES decommisionOnly:NO];
[_autofillContext removeAllObjects];
[self changeInputViewsAutofillVisibility:YES];
} else {
[_autofillContext removeAllObjects];
}

[self cleanUpViewHierarchy:YES clearText:!saveEntries];
[self cleanUpViewHierarchy:YES clearText:!saveEntries decommisionOnly:NO];
[self addToInputParentViewIfNeeded:_activeView];
}

Expand All @@ -1414,9 +1436,11 @@ - (void)setTextInputClient:(int)client withConfiguration:(NSDictionary*)configur
// Hide all input views from autofill, only make those in the new configuration visible
// to autofill.
[self changeInputViewsAutofillVisibility:NO];

// Update the current active view.
switch (autofillTypeOf(configuration)) {
case FlutterAutofillTypeNone:
self.activeView = [self updateAndShowReusableInputView:configuration];
self.activeView = [self createInputViewWith:configuration];
break;
case FlutterAutofillTypeRegular:
// If the group does not involve password autofill, only install the
Expand All @@ -1431,10 +1455,11 @@ - (void)setTextInputClient:(int)client withConfiguration:(NSDictionary*)configur
isPasswordRelated:YES];
break;
}

[_activeView setTextInputClient:client];
[_activeView reloadInputViews];

// Decommission all views that will soon be removed.
[self cleanUpViewHierarchy:NO clearText:YES decommisionOnly:YES];
// Clean up views that no longer need to be in the view hierarchy, according to
// the current autofill context. The "garbage" input views are already made
// invisible to autofill and they can't `becomeFirstResponder`, we only remove
Expand All @@ -1447,29 +1472,30 @@ - (void)setTextInputClient:(int)client withConfiguration:(NSDictionary*)configur
[self performSelector:@selector(collectGarbageInputViews) withObject:nil afterDelay:0.1];
}

// Updates and shows an input field that is not password related and has no autofill
// hints. This method re-configures and reuses an existing instance of input field
// instead of creating a new one.
// Also updates the current autofill context.
- (FlutterTextInputView*)updateAndShowReusableInputView:(NSDictionary*)configuration {
// Creates and shows an input field that is not password related and has no autofill
// hints. This method returns a new FlutterTextInputView instance when called, since
// UIKit uses the identity of `UITextInput` instances (or the identity of the input
// views) to decide whether the IME's internal states should be reset. See:
// https://github.com/flutter/flutter/issues/79031 .
- (FlutterTextInputView*)createInputViewWith:(NSDictionary*)configuration {
// It's possible that the configuration of this non-autofillable input view has
// an autofill configuration without hints. If it does, remove it from the context.
NSString* autofillId = autofillIdFromDictionary(configuration);
if (autofillId) {
[_autofillContext removeObjectForKey:autofillId];
}

[_reusableInputView configureWithDictionary:configuration];
[self addToInputParentViewIfNeeded:_reusableInputView];
_reusableInputView.textInputDelegate = _textInputDelegate;
FlutterTextInputView* newView = [[FlutterTextInputView alloc] init];
[newView configureWithDictionary:configuration];
[self addToInputParentViewIfNeeded:newView];
newView.textInputDelegate = _textInputDelegate;

for (NSDictionary* field in configuration[kAssociatedAutofillFields]) {
NSString* autofillId = autofillIdFromDictionary(field);
if (autofillId && autofillTypeOf(field) == FlutterAutofillTypeNone) {
[_autofillContext removeObjectForKey:autofillId];
}
}
return _reusableInputView;
return [newView autorelease];
}

- (FlutterTextInputView*)updateAndShowAutofillViews:(NSArray*)fields
Expand Down Expand Up @@ -1551,7 +1577,9 @@ - (UIView*)keyWindow {
// context. May remove the active view too if includeActiveView is YES.
// When clearText is YES, the text on the input fields will be set to empty before
// they are removed from the view hierarchy, to avoid triggering autofill save.
- (void)cleanUpViewHierarchy:(BOOL)includeActiveView clearText:(BOOL)clearText {
- (void)cleanUpViewHierarchy:(BOOL)includeActiveView
clearText:(BOOL)clearText
decommisionOnly:(BOOL)decommisionOnly {
Copy link
Member

@gaaclarke gaaclarke Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method would be more clear if the argument you are adding talks about "removeFromSuperview" instead of thinking about the "only" modifier on the word "decomission".

for (UIView* view in self.textInputViews) {
if ([view isKindOfClass:[FlutterTextInputView class]] &&
(includeActiveView || view != _activeView)) {
Expand All @@ -1560,14 +1588,17 @@ - (void)cleanUpViewHierarchy:(BOOL)includeActiveView clearText:(BOOL)clearText {
if (clearText) {
[inputView replaceRangeLocal:NSMakeRange(0, inputView.text.length) withText:@""];
}
[view removeFromSuperview];
[inputView decommision];
if (!decommisionOnly) {
[inputView removeFromSuperview];
}
}
}
}
}

- (void)collectGarbageInputViews {
[self cleanUpViewHierarchy:NO clearText:YES];
[self cleanUpViewHierarchy:NO clearText:YES decommisionOnly:NO];
}

// Changes the visibility of every FlutterTextInputView currently in the
Expand All @@ -1582,7 +1613,12 @@ - (void)changeInputViewsAutofillVisibility:(BOOL)newVisibility {
}

// Resets the client id of every FlutterTextInputView in the view hierarchy
// to 0. Called when a new text input connection will be established.
// to 0.
// Called before establishing a new text input connection.
// For views in the current autofill context, they need to
// stay in the view hierachy but should not be allowed to
// send messages (other than autofill related ones) to the
// framework.
- (void)resetAllClientIds {
for (UIView* view in self.textInputViews) {
if ([view isKindOfClass:[FlutterTextInputView class]]) {
Expand Down
33 changes: 29 additions & 4 deletions shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ - (void)setEditableTransform:(NSArray*)matrix;
- (void)setTextInputState:(NSDictionary*)state;
- (void)setMarkedRect:(CGRect)markedRect;
- (void)updateEditingState;
- (void)decommisson;
- (BOOL)isVisibleToAutofill;

@end
Expand Down Expand Up @@ -252,6 +253,34 @@ - (void)testInputViewCrash {
[activeView updateEditingState];
}

- (void)testDoNotReuseInputViews {
NSDictionary* config = self.mutableTemplateCopy;
[self setClientId:123 configuration:config];
FlutterTextInputView* currentView = textInputPlugin.activeView;
[self setClientId:456 configuration:config];

XCTAssertNotNil(currentView);
XCTAssertNotNil(textInputPlugin.activeView);
XCTAssertNotEqual(currentView, textInputPlugin.activeView);
}

- (void)testNoDanglingEnginePointer {
NSDictionary* config = self.mutableTemplateCopy;
[self setClientId:123 configuration:config];

// We'll hold onto the current view and try to access the engine
// after changing the active view.
FlutterTextInputView* currentView = textInputPlugin.activeView;
[self setClientId:456 configuration:config];
XCTAssertNotNil(currentView);
XCTAssertNotNil(textInputPlugin.activeView);
XCTAssertNotEqual(currentView, textInputPlugin.activeView);

// Verify that the view can no longer access the engine
// instance.
XCTAssertNil(currentView.textInputDelegate);
}

- (void)ensureOnlyActiveViewCanBecomeFirstResponder {
for (FlutterTextInputView* inputView in self.installedInputViews) {
XCTAssertEqual(inputView.canBecomeFirstResponder, inputView == textInputPlugin.activeView);
Expand Down Expand Up @@ -673,7 +702,6 @@ - (void)testCommitAutofillContext {
[self ensureOnlyActiveViewCanBecomeFirstResponder];

[self commitAutofillContextAndVerify];
XCTAssertNotEqual(textInputPlugin.textInputView, textInputPlugin.reusableInputView);
[self ensureOnlyActiveViewCanBecomeFirstResponder];

// Install the password field again.
Expand All @@ -689,14 +717,12 @@ - (void)testCommitAutofillContext {
[self ensureOnlyActiveViewCanBecomeFirstResponder];

[self commitAutofillContextAndVerify];
XCTAssertNotEqual(textInputPlugin.textInputView, textInputPlugin.reusableInputView);
[self ensureOnlyActiveViewCanBecomeFirstResponder];

// Now switch to an input field that does not autofill.
[self setClientId:125 configuration:self.mutableTemplateCopy];

XCTAssertEqual(self.viewsVisibleToAutofill.count, 0);
XCTAssertEqual(textInputPlugin.textInputView, textInputPlugin.reusableInputView);
// The active view should still be installed so it doesn't get
// deallocated.

Expand All @@ -706,7 +732,6 @@ - (void)testCommitAutofillContext {
[self ensureOnlyActiveViewCanBecomeFirstResponder];

[self commitAutofillContextAndVerify];
XCTAssertEqual(textInputPlugin.textInputView, textInputPlugin.reusableInputView);
[self ensureOnlyActiveViewCanBecomeFirstResponder];
}

Expand Down