Closed Bug 850672 Opened 9 years ago Closed 9 years ago

use-after-poison with tables, -moz-perspective and transform [@ OverflowChangedTracker::Flush]

Categories

(Core :: Layout, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox21 --- unaffected
firefox22 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: miaubiz, Assigned: mattwoodrow)

References

Details

(5 keywords, Whiteboard: [asan][adv-main22-])

Attachments

(4 files, 1 obsolete file)

Attached file repro case
this: 

<html>
  <head>
    <style>
      #el0 {
        display: table-cell;
      }
      #el2 { 
        transform: skew(0);
        display: table;
        float: left;
      }
      #el1 { 
        float: left;
      } 
      .c0 { 
        float: left;
      } 
    </style>
    <script>
      onload = function() {
        el0=document.createElement('div')
        el0.setAttribute('id','el0')
        document.body.appendChild(el0)
        el1=document.createElement('div')
        el1.setAttribute('id','el1')
        document.body.appendChild(el1)
        el2=document.createElement('div')
        el2.setAttribute('id','el2')
        document.body.appendChild(el2)
        document.body.offsetTop
        document.styleSheets[0].insertRule("#el2 { -moz-perspective: 1px; } ", 0)
        el0.setAttribute('class', 'c0') 
        document.body.offsetTop
        }</script>
      </head><body>
  </body></html>


gives this:

==70831== ERROR: AddressSanitizer: use-after-poison on address 0x00011f429b98 at pc 0x104207bfb bp 0x7fff5fbeb570 sp 0x7fff5fbeb568
READ of size 8 at 0x00011f429b98 thread T0
    #0 0x104207bfa in nsIFrame::StyleContext() const (in XUL) + 42
    #1 0x104207af8 in nsIFrame::PresContext() const (in XUL) + 8
    #2 0x1042b3671 in nsIFrame::Properties() const (in XUL) + 113

0x00011f429b98 is located 7000 bytes inside of 8192-byte region [0x00011f428040,0x00011f42a040)
allocated by thread T0 here:
    #0 0x10000ec7c in 0x20000ec7c
    #1 0x7fff93066152 in _asl_msg_test_expression (in libsystem_c.dylib) + 82
    #2 0x7fff93066ba6 in _asl_msg_new_key_val_op (in libsystem_c.dylib) + 570
    #3 0x1025a4e9a in PL_ArenaAllocate (in libplds4.dylib) + 682
Attached file asan log osx
Attached file asan log linux
mOverflowChangedTracker contains a destroyed frame.  We do get a call to
nsCSSFrameConstructor::NotifyDestroyingFrame for that frame while it is in
the mOverflowChangedTracker list, but it appears the RemoveFrame call there
fails to remove it for some reason.
Severity: normal → critical
OS: Mac OS X → All
Hardware: x86 → All
Summary: use-after-poison with tables, -moz-perspective and transform → use-after-poison with tables, -moz-perspective and transform [@ OverflowChangedTracker::Flush]
Whiteboard: [asan]
Assignee: nobody → matt.woodrow
Assignee: matt.woodrow → matspal
Well, I got as far as OverflowChangedTracker::RemoveFrame
http://hg.mozilla.org/mozilla-central/annotate/b2636816c7fd/layout/base/RestyleTracker.h#l59
Is depth==0 supposed to find the entry for the frame anywhere in the SplayTree?

I think Matt is a better owner for this bug, he wrote all of this code afaict.
Assignee: matspal → matt.woodrow
Our current understanding is that frame-poisoning is an effective mitigation so not awarding a bug bounty
Flags: sec-bounty? → sec-bounty-
Attached patch Fix RemoveFrame (obsolete) — Splinter Review
Good catch Mats, that is exactly the issue.

This is a regression from bug 849263 I guess, since before that we were pretty much ignoring the depth parameter.
Attachment #727019 - Flags: review?(roc)
Blocks: 849263
Entry(aFrame, false) expands to Entry(aFrame, aFrame->GetDepthInFrameTree(), false).
Would it be worth trying to avoid doing GetDepthInFrameTree() twice?
Also, this is called unconditionally on every destroyed frame so it might be
worth doing an early return if mEntryList is empty? (to avoid doing
GetDepthInFrameTree() at all).
(In reply to Mats Palmgren [:mats] from comment #7)
> Entry(aFrame, false) expands to Entry(aFrame, aFrame->GetDepthInFrameTree(),
> false).
> Would it be worth trying to avoid doing GetDepthInFrameTree() twice?

Yes please

> Also, this is called unconditionally on every destroyed frame so it might be
> worth doing an early return if mEntryList is empty? (to avoid doing
> GetDepthInFrameTree() at all).

Yes!
Attached patch Fix RemoveFrameSplinter Review
Good call. I took the liberty of fixing AddFrame in the same way too
Attachment #727019 - Attachment is obsolete: true
Attachment #727019 - Flags: review?(roc)
Attachment #727035 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/b9f77eff4526
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Whiteboard: [asan] → [asan][adv-main22-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.