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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Chris Peterson [:cpeterson]]

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

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: part-1-extract-Span-class.patch

Part 1: Extract selection index juggling into a Span helper class.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-2-clamp-setSelection-parameters.patch

Part 2: Clamp bogus selection indexes passed to setSelection().

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: part-3-clamp-setComposingRegion.patch

Part 3: Clamp bogus composing region indexes passed to setComposingRegion().

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: part-4-clamp-getTextBeforeAfterCursor.patch

Part 4: Clamp string lengths passed to getTextBeforeCursor/getTextAfterCursor.

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: part-5-clamp-composing-span.patch

Part 5: Clamp composing span indexes.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 8

[Chris Peterson [:cpeterson]]

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

Comment 9

[Chris Peterson [:cpeterson]]

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

Comment 10

[Chris Peterson [:cpeterson]]

Attachment: part-3-clamp-setComposingRegion-v2.patch

Part 3 v2: replace setComposingRegion()'s call to `new Span()` with `Span.clamp()` named constructor.

Comment 11

[Chris Peterson [:cpeterson]]

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

Comment 12

[Ed Morley [:emorley]]

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

Comment 13

[Scoobidiver (away)]

Crashes that depends on this bug are #19 top crasher in 14.0.

Comment 14

[Aaron Train [:aaronmt]]

Is this on Beta?

Comment 15

[Chris Peterson [:cpeterson]]

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.

Comment 16

[Aaron Train [:aaronmt]]

Ok, sounds like wont-fix for 15.