(Thus far I've only verified this bug in AVMShell as debugging it is quite annoying. Adobe should be able to verify it very quickly though or reply that it's fixed so I'm reporting it as-is ATM. I'm working on a repro for Flash and I'll update the bug if I get it working.)

JSONClass.cpp has the following comment and class definition:

***********
        // The purpose of AutoDestructingAtomArray is to build up a
        // worklist of property names to traverse during stringify.
        // (A hypothetical direct traversal that recursively processes
        //  the properties easily hits stack overflow problems.)
        //
        // Note that it allocates the worklist via the given FixedMalloc
        // allocator and automatically frees the worklist during destruction;
        // thus the stringify kernel must be careful to catch any AS3
        // exception from a callback and unroll our stack properly
        // before rethrowing the exception.
        //
        // Note also that it is an anti-pattern to be storing Atoms in
        // the worklist (a FixedMalloc-allocated array) in this
        // fashion, because the worklist will not be traced by the
        // garbage collector.  The argument justifying it in this case
        // is that all of these atoms are property names that are kept
        // alive by the object we are stringifying, but that is not
        // a great argument.  Also this code would completely break
        // in the presence of a copying garbage collector.
        struct AutoDestructingAtomArray {
            AutoDestructingAtomArray(MMgc::FixedMalloc* fm, int32_t atomCount)
                : m_atoms(NULL)
                , m_atomCount(atomCount)
                , m_fixedmalloc(fm)
            {
                if (atomCount > 0)
                    m_atoms = (Atom*)fm->Alloc(atomCount*sizeof(Atom));
            }

            ~AutoDestructingAtomArray() {
                if (m_atomCount > 0)
                    m_fixedmalloc->Free(m_atoms);
            }

            Atom*              m_atoms;
            int32_t            m_atomCount;
            MMgc::FixedMalloc* m_fixedmalloc;
        };
***********

The class is a wrapper around a FixedMalloc allocated buffer holding Atoms.
As the comment points out, this is a potentially very dangerous anti-pattern;
FixedMalloc allocations are not GC roots and as such won't be traced during a GC mark phase.
The Atom type is a tagged-pointer which can point to GC-ed objects, therefore *all* Atoms
must be either reachable via a GC-root or, if they aren't rooted, then only exist unrooted
when it can be guaranteed that no GC can occur (since a GC could collect the underlying object
which the Atom points to.)

The comment preceeding AutoDestructingAtomArray claims that "all of these atoms are property names
that are kept alive by the object we are stringifying". This isn't true.

The propNames.m_atoms array is initialized here:

        index = value->nextNameIndex(0);
        int32_t propNamesIdx = 0;
        while (index != 0) {
            Atom name = value->nextName(index);
            propNames.m_atoms[propNamesIdx] = name;
            propNamesIdx++;
            index = value->nextNameIndex(index);
        }

and then used in this loop later in the code:

        for (propNamesIdx = 0; propNamesIdx < ownDynPropCount; propNamesIdx++) {
            Atom name = propNames.m_atoms[propNamesIdx];
            if (!AvmCore::isString(name) ||
                value->getAtomPropertyIsEnumerable(name)) {
                ReturnCondition ret;
                ret = JOProp(name, value, pendingPre);
                RET_EXN_CHECK(ret);
                if (ret == kSomeOutput) {
                    pendingPre = connective;
                    emittedSomething = true;
                }
            }
        }

The comment claimed that the Atoms in m_atoms are kept alive by the object being stringified but it
failed to take into account that objects can override the "toJSON" method, and therefore when a property
is stringified it can run arbitrary AS3 code in the (reached here via JOProp) which can mutate other objects' property hashtables.

In order for those property names to be collected it's important that the object property hashtable is the
only reference to the property name atoms therefore in this example we create an object with 100 properties which are
created dynamically using String.fromCharCode(). We then add extra properties "foo" and "baz", the values of which are
an object which overrides toJSON to delete those 100 properties and then force a GC to ensure that the underlying
objects are free'd.

After either the foo or baz properties have been stringified the Atoms in the propNames.m_atoms array point
to objects which have been free'd.

The getAtomPropertyIsEnumerable method will call getIntAtom which writes to the underlying object, therefore if the
underlying object was reallocated this could almost certainly lead to exploitable memory corruption.

With this PoC you will be able to observe in a debugger
that the Atoms have been collected and the objects have been freed and the memory memset to 0.

In a debug build of AVMShell this will lead to an AvmAssert failing in getAtomPropertyIsEnumerable.

I am still working on a PoC which will crash flash player, debugging the GC is much harder in a non-debug build.
 

stringify_gc_report.as 6.2 KB Download