Last Comment Bug 765831 - Clamp more IME selection and composing region indexes that may overrun our editable content
: Clamp more IME selection and composing region indexes that may overrun our ed...
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Keyboards and IME (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 16
Assigned To: Chris Peterson [:cpeterson]
:
Mentors:
Depends on:
Blocks: 760396 733138 747629
  Show dependency treegraph
 
Reported: 2012-06-18 10:50 PDT by Chris Peterson [:cpeterson]
Modified: 2013-12-10 10:01 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
wontfix
fixed


Attachments
part-1-extract-Span-class.patch (10.43 KB, patch)
2012-06-18 10:54 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review+
Details | Diff | Splinter Review
part-2-clamp-setSelection-parameters.patch (1.58 KB, patch)
2012-06-18 10:54 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review+
Details | Diff | Splinter Review
part-3-clamp-setComposingRegion.patch (1.20 KB, patch)
2012-06-18 10:55 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review-
Details | Diff | Splinter Review
part-4-clamp-getTextBeforeAfterCursor.patch (2.02 KB, patch)
2012-06-18 10:56 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review+
Details | Diff | Splinter Review
part-5-clamp-composing-span.patch (5.25 KB, patch)
2012-06-18 10:57 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review+
Details | Diff | Splinter Review
part-3-clamp-setComposingRegion-v2.patch (1.20 KB, patch)
2012-06-19 12:19 PDT, Chris Peterson [:cpeterson]
blassey.bugs: review+
Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2012-06-18 10:50:40 PDT
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] 2012-06-18 10:54:04 PDT
Created attachment 634107 [details] [diff] [review]
part-1-extract-Span-class.patch

Part 1: Extract selection index juggling into a Span helper class.
Comment 2 Chris Peterson [:cpeterson] 2012-06-18 10:54:35 PDT
Created attachment 634108 [details] [diff] [review]
part-2-clamp-setSelection-parameters.patch

Part 2: Clamp bogus selection indexes passed to setSelection().
Comment 3 Chris Peterson [:cpeterson] 2012-06-18 10:55:14 PDT
Created attachment 634109 [details] [diff] [review]
part-3-clamp-setComposingRegion.patch

Part 3: Clamp bogus composing region indexes passed to setComposingRegion().
Comment 4 Chris Peterson [:cpeterson] 2012-06-18 10:56:37 PDT
Created attachment 634110 [details] [diff] [review]
part-4-clamp-getTextBeforeAfterCursor.patch

Part 4: Clamp string lengths passed to getTextBeforeCursor/getTextAfterCursor.
Comment 5 Chris Peterson [:cpeterson] 2012-06-18 10:57:11 PDT
Created attachment 634111 [details] [diff] [review]
part-5-clamp-composing-span.patch

Part 5: Clamp composing span indexes.
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2012-06-18 16:11:11 PDT
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?) 2012-06-18 16:12:55 PDT
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] 2012-06-19 11:00:16 PDT
(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] 2012-06-19 11:02:19 PDT
(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] 2012-06-19 12:19:07 PDT
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.
Comment 13 Scoobidiver (away) 2012-06-30 00:16:38 PDT
Crashes that depends on this bug are #19 top crasher in 14.0.
Comment 14 Aaron Train [:aaronmt] 2012-07-24 11:27:58 PDT
Is this on Beta?
Comment 15 Chris Peterson [:cpeterson] 2012-07-24 11:57:53 PDT
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] 2012-07-24 13:10:07 PDT
Ok, sounds like wont-fix for 15.

Note You need to log in before you can comment on or make changes to this bug.