555614 - Assertion failed: "((index < len))" ("/Users/edwin/vm/argo/core/avmplusList.h":397)

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Edwin Smith]

reading past end of List<> causes assert:

Assertion failed: "((index < len))" ("/Users/edwin/vm/argo/core/avmplusList.h":397)

happens in getClassTraits() called from parseInstanceInfos in AbcParser.  The code correctly does a range check first, but at this point the list of class traits has length 0.  It should have been pre-allocated to classCount entries.

        // get the class type
        if (class_index >= classCount)
            toplevel->throwVerifyError(kClassInfoExceedsCountError,
               core->toErrorString(class_index), core->toErrorString(classCount));

=>          Traits* ctraits = pool->getClassTraits(class_index);
            if (!ctraits)
	        toplevel->throwVerifyError(kClassInfoOrderError,
                   core->toErrorString(class_index));

Comment 1

[Edwin Smith]

Attachment: fuzzed abcdump.abc test case

Comment 2

[Edwin Smith]

The test case also causes abcdump to fail with:

+TypeError: Error #1009: Cannot access a property or method of a null object reference.
+	at Abc/parseTraits()[abcdump.as:1047]
+	at Abc/parseInstanceInfos()[abcdump.as:1017]
+	at Abc()[abcdump.as:720]
+	at global$init()[abcdump.as:1518]

presumably this is abcdump doing a null pointer derefernce of its classes array.

Comment 3

[Edwin Smith]

Attachment: range-check class_index against actual class list length, not just max classCount

The assert fires because we access List<> past length.  The list is correctly presized, so the overrun is not actually a memory safety issue.  The list is GC allocated and so non-initialized entries are guaranteed to be null.

patch does the range check explicitly.  even though without the assert the code is memory safe, its too fragile and it violates the List<> api contract.

Comment 4

[Edwin Smith]

Targetting 10.1, although not strictly required, because the fix is simple and aids testing.

* still needs a test case
* abcdump still needs a fix.

Comment 5

[Tom Harwood]

Comment on attachment 435985 [details] [diff] [review]
range-check class_index against actual class list length, not just max classCount

Looks good.  I'd bet similar issues exist in some other lists.

Comment 6

[Edwin Smith]

pushed to tr-argo and tr
http://hg.mozilla.org/tamarin-redux/rev/5403aeeddc19

Comment 7

[Chris Peyer]

Pushed regression testcase (attachment 435509 [details]) to redux changeset 4432 e8a4288bf822