Clamp more IME selection and composing region indexes that may overrun our editable content

RESOLVED FIXED in Firefox 16

Status

()

Firefox for Android
Keyboards and IME
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 16
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 affected, firefox15 wontfix, firefox16 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
After reviewing IME crash stats, I found some more GeckoInputConnection methods that may be called with out-of-bounds indexes from IMEs.
(Assignee)

Comment 1

5 years ago
Created attachment 634107 [details] [diff] [review]
part-1-extract-Span-class.patch

Part 1: Extract selection index juggling into a Span helper class.
Attachment #634107 - Flags: review?(blassey.bugs)
(Assignee)

Comment 2

5 years ago
Created attachment 634108 [details] [diff] [review]
part-2-clamp-setSelection-parameters.patch

Part 2: Clamp bogus selection indexes passed to setSelection().
Attachment #634108 - Flags: review?(blassey.bugs)
(Assignee)

Comment 3

5 years ago
Created attachment 634109 [details] [diff] [review]
part-3-clamp-setComposingRegion.patch

Part 3: Clamp bogus composing region indexes passed to setComposingRegion().
Attachment #634109 - Flags: review?(blassey.bugs)
(Assignee)

Comment 4

5 years ago
Created attachment 634110 [details] [diff] [review]
part-4-clamp-getTextBeforeAfterCursor.patch

Part 4: Clamp string lengths passed to getTextBeforeCursor/getTextAfterCursor.
Attachment #634110 - Flags: review?(blassey.bugs)
(Assignee)

Comment 5

5 years ago
Created attachment 634111 [details] [diff] [review]
part-5-clamp-composing-span.patch

Part 5: Clamp composing span indexes.
Attachment #634111 - Flags: review?(blassey.bugs)
Comment on attachment 634107 [details] [diff] [review]
part-1-extract-Span-class.patch

Review of attachment 634107 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoInputConnection.java
@@ +199,5 @@
>                  // If there is no selection set, we must be doing "Copy All",
>                  // otherwise get the selection
> +                if (selection.length > 0)
> +                    text = text.substring(selection.start, selection.end);
> +                GeckoAppShell.setClipboardText(text);

I don't think this is right. Before if a were equal to b we would send an empty string. Now we'll send whatever is in text.
Attachment #634107 - Flags: review?(blassey.bugs) → review+
Attachment #634108 - Flags: review?(blassey.bugs) → review+
Comment on attachment 634109 [details] [diff] [review]
part-3-clamp-setComposingRegion.patch

Review of attachment 634109 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoInputConnection.java
@@ +392,5 @@
>              endComposition();
>          }
>  
> +        Span newComposingRegion = new Span(getEditable(), start, end);
> +        return super.setComposingRegion(newComposingRegion.start, newComposingRegion.end);

what does this accomplish? Needs a comment at least.
Attachment #634109 - Flags: review?(blassey.bugs) → review-
Attachment #634110 - Flags: review?(blassey.bugs) → review+
Attachment #634111 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 8

5 years ago
(In reply to Brad Lassey [:blassey] from comment #6)
> ::: mobile/android/base/GeckoInputConnection.java
> @@ +199,5 @@
> >                  // If there is no selection set, we must be doing "Copy All",
> >                  // otherwise get the selection
> > +                if (selection.length > 0)
> > +                    text = text.substring(selection.start, selection.end);
> > +                GeckoAppShell.setClipboardText(text);
> 
> I don't think this is right. Before if a were equal to b we would send an
> empty string. Now we'll send whatever is in text.

The comment says no selection implies "Copy All", but that differs from the existing code. So I will correct the comment.
(Assignee)

Comment 9

5 years ago
(In reply to Brad Lassey [:blassey] from comment #7)
> > +        Span newComposingRegion = new Span(getEditable(), start, end);
> > +        return super.setComposingRegion(newComposingRegion.start, newComposingRegion.end);
> 
> what does this accomplish? Needs a comment at least.

The Span ctor will clamp the start/end parameters, but that behavior is not obvious from the calling code. I will change the Span class to use a "named constructor" called Span.clamp() that will make the caller's intent clear.
(Assignee)

Comment 10

5 years ago
Created attachment 634537 [details] [diff] [review]
part-3-clamp-setComposingRegion-v2.patch

Part 3 v2: replace setComposingRegion()'s call to `new Span()` with `Span.clamp()` named constructor.
Attachment #634109 - Attachment is obsolete: true
Attachment #634537 - Flags: review?(blassey.bugs)
Attachment #634537 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/97bc7b4de4ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba96999567ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7def2566200
https://hg.mozilla.org/integration/mozilla-inbound/rev/80528404a24e
https://hg.mozilla.org/integration/mozilla-inbound/rev/62d5b893232b
status-firefox16: affected → fixed
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/97bc7b4de4ff
https://hg.mozilla.org/mozilla-central/rev/ba96999567ad
https://hg.mozilla.org/mozilla-central/rev/c7def2566200
https://hg.mozilla.org/mozilla-central/rev/80528404a24e
https://hg.mozilla.org/mozilla-central/rev/62d5b893232b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 733138

Comment 13

5 years ago
Crashes that depends on this bug are #19 top crasher in 14.0.
tracking-fennec: --- → ?
Is this on Beta?
(Assignee)

Comment 15

5 years ago
Aaron, these fixes are Aurora 16, but not Beta 15. At the time, I felt they were too risky to uplift. 

In Firefox 14.0.1's 

In recent crash stats, these IME crashes are ~186th place for Firefox 14.0.1 and ~49 for Beta 15, so I don't think they are a high priority for uplifting.
Ok, sounds like wont-fix for 15.
status-firefox15: affected → wontfix
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.