Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding

RESOLVED FIXED in Firefox 40, Firefox OS v2.0

Status

()

Core
Widget
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: poiru, Assigned: poiru)

Tracking

({sec-audit})

unspecified
mozilla42
sec-audit
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 wontfix, firefox40+ fixed, firefox41+ fixed, firefox42+ fixed, firefox-esr31 wontfix, firefox-esr3840+ 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)

Details

(Whiteboard: [post-critsmash-triage][adv-main40-][adv-esr38.2-])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8636104 [details] [diff] [review]
Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding
Attachment #8636104 - Flags: review?(evilpies)
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.
(Assignee)

Comment 5

2 years ago
(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.
(Assignee)

Comment 7

2 years ago
Created attachment 8636410 [details] [diff] [review]
Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding
Attachment #8636410 - Flags: review?(masayuki)
(Assignee)

Updated

2 years ago
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.
status-firefox39: --- → wontfix
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox-esr31: --- → wontfix
status-firefox-esr38: --- → affected
tracking-firefox40: --- → +
tracking-firefox41: --- → +
tracking-firefox42: --- → +
tracking-firefox-esr38: --- → 40+
Keywords: sec-audit
(Assignee)

Comment 10

2 years ago
Created attachment 8636668 [details] [diff] [review]
esr38 patch

[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?
(Assignee)

Comment 11

2 years ago
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
Last Resolved: 2 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
https://hg.mozilla.org/releases/mozilla-aurora/rev/eb80ce0eb9c8
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-v2.2r: --- → affected
status-b2g-master: --- → fixed
status-firefox41: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/4febcc97a5d6
status-firefox40: affected → fixed
https://hg.mozilla.org/releases/mozilla-esr38/rev/f6da6a262fca
status-firefox-esr38: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/da1458e68958
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/da1458e68958
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/def978d33afc
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/def978d33afc
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/90e98e683875
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/90e98e683875
status-b2g-v2.0: affected → fixed
status-b2g-v2.0M: affected → fixed
status-b2g-v2.1: affected → fixed
status-b2g-v2.1S: affected → fixed
status-b2g-v2.2: affected → fixed
status-b2g-v2.2r: affected → fixed
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main40-][adv-esr38.2-]

Updated

2 years ago
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.