bugGnash - The GNU Flash player - Bugs: bug #39385, ...

 
 

bug #39385: boost/variant/detail/visitation_impl.hpp:207: ... Assertion `false' failed.

Submitted by:  Bastiaan Jacques <bjacques>
Submitted on:  Mon 01 Jul 2013 12:19:19 PM UTC  
 
Category: coreSeverity: 6 - Security
Release: masterStatus: Fixed
Privacy: PublicAssigned to: Bastiaan Jacques <bjacques>
Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Sat 13 Jul 2013 10:45:35 AM UTC, comment #2:

Test added in 906e67a84, fixed in d920f0656.

Bastiaan Jacques <bjacques>
Project MemberIn charge of this item.
Tue 02 Jul 2013 02:23:45 PM UTC, comment #1:

I'd go with switching to std::list::sort if possible, sounds like the safest thing to do, but this will need:

1) a testcase exposing the abort with current code
2) verification that all other existing tests work the same with the fix in place

Thank you for the excellent analysis, btw :)

Sandro Santilli <strk>
Project Member
Mon 01 Jul 2013 12:19:19 PM UTC, original submission:

Please see downstream report: https://bugzilla.redhat.com/show_bug.cgi?id=924339

The abort can be reproduced reliably by starting Gnash with the noted arguments, omitting the ones relating to embedding Gnash.

Although a boost assertion fails, I quickly realised that some sort of memory corruption is happening, leading to Boost balking on an invalid variant state. The source of the problem appears to be sortIndexed() (Array_as.cpp), but an inspection of the array that was being sorted yielded no sources of invalid memory.

In this case, an array is being sorted using a scripted comparator. I soon discovered that replacing the comparator prevented the problem altogether. The offending comparator reads as follows:

Unfortunately, C++ requires that the comparator be modeled after 'strict weak ordering'. If this requirement is not fulfilled, corruption problems appear (see e.g. http://schneide.wordpress.com/2010/11/01/bug-hunting-fun-with-stdsort/).

In this case, this Flash movie's author's intention is that the array is randomized rather than sorted, and obviously the comparator written does not follow any ordering model.

The bug can be reduced to this testcase:

(If the second argument is omitted, the bug does not appear.)

For reference I looked at Spidermonkey, where a custom implementation of MergeSort is used for scripted comparators, which is evidently insensitive to badly written comparators.

In Gnash, the author of the array sorting code seems to have been aware of this problem, where in one of the two sorting methods in Array_as.cpp it is noted that:

The author seems to have worked around this issue by employing std::list::sort(), which does not seem to exhibit detectable memory problems when using a nonconforming comparator. (ISO/IEC
14882 nonetheless states that std::list::sort()'s comparator must also meet the strict weak ordering model.)

In the case of this bug the codepath leads to a different sorting function, which uses std::sort().

It seems to me that the quick fix here is to alter the second sorting function to also use std::list::sort(). In the long run we should consider rolling our own sort algorithm, so that we can guarantee memory safety, if not useful results, with nonconforming comparators.

Opinions are of course welcome.

Bastiaan Jacques <bjacques>
Project MemberIn charge of this item.

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by strk (Posted a comment)
  • -unavailable- added by bjacques (Submitted the item)
  • -unavailable- added by bjacques
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sat 13 Jul 2013 10:45:35 AM UTCbjacquesStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Fri 12 Jul 2013 10:14:17 PM UTCbjacquesStatusConfirmed=>Ready For Test
      Assigned toNone=>bjacques
    Mon 01 Jul 2013 12:19:19 PM UTCbjacquesCarbon-Copy-=>Added bwy

    Back to the top


    Powered by Savane 3.1-cleanup1