Closed Bug 373610 (refdyn) Opened 18 years ago Closed 2 years ago

[meta] Bugs found by comparing renderings with and without dynamic changes

Categories

(Core :: Fuzzing, defect)

Product:
Core
Component:
Fuzzing
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jruderman, Unassigned)

References

(Depends on 55 open bugs)

Details

(Keywords: meta)

Attachments

(5 obsolete files) 

This reftest-based script exhaustively tests how rendering responds to simple DOM changes.  For example, it tries removing a node, forcing a complete relayout of the page, and putting the node back.    It compares the two screenshots with the node missing to ensure that the removeChild operation is handled properly, and compares the two screenshots with the node present to ensure that the insertBefore operation is handled properly.

It tests removeChild/insertBefore for every node in the document.  It also tests attribute removal/setting and text node changes.

Running it on all the reftest files (excluding the pixel-rounding directory) takes about 10 hours on my MacBook Pro, so it's too slow to be run automatically after every checkin.  About 80% of the time is spent in PNG compression.  Is there a way to avoid PNG compression in this kind of test?

It has found 15 bugs so far, mostly incorrect-rendering bugs and a few bugs in reading the style attribute.  But I'm filing this as security-sensitive for now because I'm worried it might find more severe bugs on branches and because my experience with bug 349611 makes me feel I should be cautious.
Attached file refdyn.js (obsolete) — 

    
  
Whiteboard: [sg:nse meta]

    
    

    
  
This is a great idea.
It seems to me, a lot of bugs could also be found by comparing the offsetWidth/offsetHeight/computed width/computed height, not?
In that case, you could avoid the slow image comparison.

    
    

    
  
Jesse, even if it takes 10 hours, we could (and probably should) still run the test nightly if we had appropriate automation. cc'ing pav/vlad about avoiding PNG compression. It probably wouldn't be that hard to add a parameter to avoid compression.

    
    

    
  
cool idea, Jesse. We should figure out how to get this running regularly on the test farm. I'll talk to you later about some ideas on irc.

    
    

    
  
The best way to avoid the PNG compression would be to write a small image comparison function in C++ rather than doing the image comparison via PNG data: URLs.  It's something that's on my mental todo list for speeding up reftest, though there should probably be a bug filed on it.  (We probably would still want to convert to data: URLs when reporting failures, but that's rare.)

    
    

    
  
Can the image comparison be done with getImageData and in JS?  Not sure what the perf would be like, but something to consider would be to use a much smaller basea size for the tests, or at least have a test flag that indicates that a test needs a large canvas vs. the default (which should be small, like 100x100).  A C++ thing could work fine as well.

    
    

    
  
I noticed in reftest.js that setAttribute is explicitly called on the Canvas to set its height and width. Does this necessarily need to be set to the size of the window, or can it be set to capture only a small square of the window? Or, can the width of the window be set here

If this size could be made a parameter of the test, it would be most convenient.

    
  
Alias: refdyn

    
  
Depends on: 375493

    
  
Depends on: 375497

    
  
Depends on: 381277

    
  
Depends on: 381279

    
  
Depends on: 382146

    
  
Depends on: 385607

    
    

    
  

      
      
      
      

        Attached file
          
        refdyn.js (obsolete)
        — 
      

    


  

        Attachment #258275 -
        Attachment is obsolete: true

    
  
Depends on: 387510

    
  
Depends on: 395125

    
  
Depends on: 395130

    
  
Depends on: 377519

    
  
Depends on: 395155

    
  
Depends on: 398092

    
  
Depends on: 398095

    
  
Depends on: 398101

    
  
Depends on: 398105

    
    

    
  

      
      
      
      

        Attached file
          
        refdyn.zip (obsolete)
        — 
      

    


  
Now much faster thanks to bug 387132.  Also contains updated exclusions.

        Attachment #270363 -
        Attachment is obsolete: true

    
  
Depends on: 398681

    
  
Depends on: 398682

    
  
Depends on: 398685

    
  
Depends on: 398686

    
  
Depends on: 398809

    
  
Depends on: 398820

    
  
Depends on: 398830

    
  
Depends on: 398884

    
  
Depends on: 401580

    
  
Depends on: 403733

    
  
Depends on: 405517

    
  
Depends on: 407095

    
  
Depends on: 407106

    
  
Depends on: 407115

    
  
Depends on: 407397

    
  
Depends on: 407419

    
  
Depends on: 408782

    
  
Depends on: 409051

    
  
Depends on: 409056

    
  
Depends on: 409065

    
  
Depends on: 409089

    
  
Depends on: 409125

    
  
Depends on: 409494

    
  
Depends on: 409577

    
    

    
  

      
      
      
      

        Attached file
          
        refdyn.zip (obsolete)
        — 
      

    


  
Updated to understand the current reftest.list format.  Many new exclusions.

        Attachment #283658 -
        Attachment is obsolete: true

    
    

    
  
The bugs that force me to exclude the most tests are:

* Bug 162063 - Tables
* Bug 409089 - -moz-box
* Bug 409125 - MathML
* Bug 229915 - CSS + combinator
* Bug 373298 - :-moz-first-node 
* Bug 145419 - dynamically added ::first-letter and ::first-line rules

    
  
Depends on: 411367

    
  
Depends on: 411374

    
  
Depends on: 412352

    
  
Depends on: 412365

    
  
Depends on: 418574

    
  
Depends on: 418756

    
  
Depends on: 418766

    
  
Depends on: 421234

    
  
Depends on: 421239

    
  
Depends on: 421419

    
  
Depends on: 421425

    
    

    
  
After finding dozens of harmless layout inconsistencies, refdyn finally found a potential security hole!  Bug 421234 causes a random character to appear, and I'm guessing it comes from uninitialized memory.

    
    

    
  

      
      
      
      

        Attached file
          
        refdyn.zip (obsolete)
        — 
      

    


  

        Attachment #294382 -
        Attachment is obsolete: true

    
  
Depends on: 423130

    
  
Depends on: 429974

    
  
Depends on: 429976

    
  
Depends on: 442630

    
  
Depends on: 442633

    
  
Depends on: 450693

    
  
Depends on: 467312

    
  
Depends on: 467321

    
  
Depends on: 467323

    
  
Depends on: 467460

    
  
Depends on: 467472

    
  
Depends on: 467498

    
  
Depends on: 467719

    
  
Depends on: 467722

    
  
Depends on: 467723

    
  
Depends on: 475642

    
  
Depends on: 475644

    
  
Depends on: 475647

    
  
Depends on: 478490

    
  
Depends on: 478511

    
  
Depends on: 478594

    
  
Depends on: 489868

    
  
Depends on: 489877

    
  
Depends on: 489887

    
  
Depends on: 489890

    
  
Depends on: 490173

    
  
Depends on: 490174

    
  
Depends on: 490176

    
  
Depends on: 490177

    
  
Depends on: 490182

    
  
Depends on: 490183

    
  
Depends on: 490185

    
  
Depends on: 490216

    
  
Depends on: 490218

    
  
Depends on: 490220

    
  
Depends on: 492231

    
  
Depends on: 492239

    
  
Depends on: 492240

    
  
Depends on: 492661

    
    

    
  


  
Refdyn has only found two security holes among about a hundred total bugs.  I'd like to make it public eventually, but one of those bugs (bug 467323) isn't fixed on 3.0.x, so it might be a while.

        Attachment #307930 -
        Attachment is obsolete: true

    
  
Depends on: 493564

    
  
Depends on: 501035

    
  
Depends on: 501037

    
  
Depends on: 501048

    
  
Depends on: 501049

    
  
Depends on: 521525

    
  
Depends on: 521527

    
  
Depends on: 521539

    
  
Depends on: 521542

    
  
Depends on: 521594

    
  
Depends on: 521600

    
  
Depends on: 521602

    
  
Depends on: 521607

    
  
Depends on: 521609

    
  
Depends on: 521682

    
  
Depends on: 521685

    
  
Depends on: 521689

    
  
Depends on: 521720

    
  
Depends on: 521875

    
  
Depends on: 522390

    
  
Depends on: 522393

    
  
Depends on: 526536

    
  
Depends on: 526596

    
  
Depends on: 526602

    
  
Depends on: 526634

    
  
Depends on: 534526

    
  
Depends on: 534793

    
  
Depends on: 534802

    
  
Depends on: 534806

    
  
Depends on: 534808

    
  
Depends on: 534811

    
  
Depends on: 537875

    
  
Depends on: 543791

    
  
Depends on: 549797

    
  
Depends on: 550047

    
  
Depends on: 550065

    
  
Depends on: 550661

    
  
Depends on: 551239

    
  
Depends on: 551838

    
    

    
  
I've revived this tool by making it part of the DOM fuzzer. Now it makes a number of random dynamic changes before checking that the resulting dynamic rendering matches the static rendering of the same DOM tree.
Group: core-security
Depends on: 721027, 720078, 720077, 718452, 718331, 718236, 718211, 718208, 718157, 718123, 718037, 717870, 723669, 723657, 723382, 723376, 723357, 726460, 726392, 725928

    
  
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [sg:nse meta]

    
  
Depends on: 726548

    
  
Depends on: 728100

    
  
Depends on: 731521

    
  
Depends on: 732740

    
  
Depends on: 752779

    
  
Depends on: 763560

    
  
Depends on: 764256

    
  
Depends on: 767233

    
  
Depends on: 767279

    
  
Depends on: 1036750

    
  
Depends on: 1156244

    
  
Depends on: 1258076

    
  
Component: Tracking → Platform Fuzzing Team

    
    

    
  
The bug assignee didn't login in Bugzilla in the last 7 months.

:decoder, could you have a look please?

For more information, please visit auto_nag documentation.


Assignee: jruderman → nobody
Flags: needinfo?(choller)
Summary: Bugs found by comparing renderings with and without dynamic changes → [meta] Bugs found by comparing renderings with and without dynamic changes

    
    

    
  
The approach here sounds similar to what Layout Quick Check does (on top of our reftests), does that sound about right? Is there anything we can learn from this older bug for our deployment / use of LQIC?


Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(choller) → needinfo?(twsmith)
Resolution: --- → FIXED
See Also:  → lqc
Flags: needinfo?(twsmith)

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: