Closed
Bug 1185589
Opened 9 years ago
Closed 9 years ago
Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla42
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)
1.88 KB,
patch
|
masayuki
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.75 KB,
patch
|
abillings
:
approval-mozilla-esr38+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Attachment #8636104 -
Flags: review?(evilpies)
Comment 2•9 years ago
|
||
Comment on attachment 8636104 [details] [diff] [review] Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding Sorry, can't review this.
Attachment #8636104 -
Flags: review?(evilpies)
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
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•9 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.
Comment 6•9 years ago
|
||
(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•9 years ago
|
||
Attachment #8636410 -
Flags: review?(masayuki)
Assignee | ||
Updated•9 years ago
|
Attachment #8636104 -
Attachment is obsolete: true
Attachment #8636104 -
Flags: review?(masayuki)
Comment 8•9 years ago
|
||
Comment on attachment 8636410 [details] [diff] [review] Fix unintentional assignment in PuppetWidget::ExecuteNativeKeyBinding Thank you!
Attachment #8636410 -
Flags: review?(masayuki) → review+
Comment 9•9 years ago
|
||
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•9 years ago
|
||
[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•9 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?
Updated•9 years ago
|
Attachment #8636410 -
Flags: approval-mozilla-beta?
Attachment #8636410 -
Flags: approval-mozilla-beta+
Attachment #8636410 -
Flags: approval-mozilla-aurora?
Attachment #8636410 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
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: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 13•9 years ago
|
||
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
Comment 16•9 years ago
|
||
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
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main40-][adv-esr38.2-]
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•