Closed Bug 1185589 Opened 4 years ago Closed 4 years ago

Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- wontfix
firefox40 + fixed
firefox41 + fixed
firefox42 + fixed
firefox-esr31 --- wontfix
firefox-esr38 40+ fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-v2.2r --- fixed
b2g-master --- fixed

People

(Reporter: poiru, Assigned: poiru)

References

Details

(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main40-][adv-esr38.2-])

Attachments

(2 files, 1 obsolete file)

Over in bug 1182723, we noticed that there seems to be an unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding. The `commands = ...` lines are actually assigning to mSingleLineCommands because `commands` is a reference to it.
Comment on attachment 8636104 [details] [diff] [review]
Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

Sorry, can't review this.
Attachment #8636104 - Flags: review?(evilpies)
Comment on attachment 8636104 [details] [diff] [review]
Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

Probably the nicest bug in Firefox that I ever produced.
Attachment #8636104 - Flags: review?(masayuki)
Comment on attachment 8636104 [details] [diff] [review]
Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

> -  nsTArray<mozilla::CommandInt>& commands = mSingleLineCommands;

I don't understand why this causes unintentional assignment. Could you explain about that?

> -  nsTArray<mozilla::CommandInt>& commands = mSingleLineCommands;
> +  const nsTArray<mozilla::CommandInt>* commands = nullptr;

...

>      case nsIWidget::NativeKeyBindingsForRichTextEditor:
> -      commands = mRichTextCommands;
> +      commands = &mRichTextCommands;
>        break;
>    }
> +  MOZ_ASSERT(commands);

I think that you actually changes the behavior. In the old code, commands references to mSingleLineCommands if aType is invalid (or defined new one but here is not maintained!) but your new code sets nullptr.

And you checks if commands is a valid pointer only on debug build. So, if actually this becomes buggy, the code will try to execute nullptr->IsEmpty() in release build.

Please add default case and insert MOZ_CRASH() in the default case.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #4)
> Comment on attachment 8636104 [details] [diff] [review]
> Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding
> 
> > -  nsTArray<mozilla::CommandInt>& commands = mSingleLineCommands;
> 
> I don't understand why this causes unintentional assignment. Could you
> explain about that?

`commands` is a reference to mSingleLineCommands. So, e.g. `commands = mMultiLineCommands` is equivalent to `mSingleLineCommands = mMultiLineCommands`, which is probably not what was intended.
(In reply to Birunthan Mohanathas [:poiru] from comment #5)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #4)
> > Comment on attachment 8636104 [details] [diff] [review]
> > Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding
> > 
> > > -  nsTArray<mozilla::CommandInt>& commands = mSingleLineCommands;
> > 
> > I don't understand why this causes unintentional assignment. Could you
> > explain about that?
> 
> `commands` is a reference to mSingleLineCommands. So, e.g. `commands =
> mMultiLineCommands` is equivalent to `mSingleLineCommands =
> mMultiLineCommands`, which is probably not what was intended.

Oh, really? I misunderstood the C++ reference's behavior.
Attachment #8636104 - Attachment is obsolete: true
Attachment #8636104 - Flags: review?(masayuki)
Comment on attachment 8636410 [details] [diff] [review]
Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

Thank you!
Attachment #8636410 - Flags: review?(masayuki) → review+
Per discussions, this is related to bug 1182723 and should go in with it on the same branches.
Attached patch esr38 patchSplinter Review
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is landing along with bug 1182723, which is a sec-high bug.
Fix Landed on Version: 42
Risk to taking this patch (and alternatives if risky): Minimal
String or UUID changes made by this patch: No
Attachment #8636668 - Flags: approval-mozilla-esr38?
Comment on attachment 8636410 [details] [diff] [review]
Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

See comment 10.
Attachment #8636410 - Flags: approval-mozilla-beta?
Attachment #8636410 - Flags: approval-mozilla-aurora?
Attachment #8636410 - Flags: approval-mozilla-beta?
Attachment #8636410 - Flags: approval-mozilla-beta+
Attachment #8636410 - Flags: approval-mozilla-aurora?
Attachment #8636410 - Flags: approval-mozilla-aurora+
Attachment #8636668 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
https://hg.mozilla.org/mozilla-central/rev/bcf65c04b69c
https://hg.mozilla.org/mozilla-central/rev/b38a3f669d49
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main40-][adv-esr38.2-]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.