555705 - Memory leaked when reporting a Verify Error

Status: VERIFIED FIXED

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

Description

[Edwin Smith]

Many fuzzed files trigger a verify error.  Some of them then go on to report a memory leak using a GC assert.  (69 dups on fuzzed hello2.abc)

Assertion failed: "((leakedBytes == 0 || GetStatus() == kMemAbort))" ("/Users/edwsmith/vm/argo-sec/MMgc/GCHeap.cpp":254) Leaks!

Comment 1

[Edwin Smith]

Attachment: fuzzed hello2.abc

One of 69 examples that leaks memory when throwing a verify error.

Comment 2

[Lars T Hansen]

At least one leak appears to be FrameState objects allocated by checkParams; the exception handler for the verifier calls Verifier.~Verifier() but that only performs a shallow cleanup of the 'state' structure, not the - presumably necessary - deep cleanup.

Comment 3

[Lars T Hansen]

The other thing that's leaked in this case is the 'locals' array allocated by FrameState::FrameState, which is natural.

Comment 4

[Lars T Hansen]

Abstraction breakage.

MethodInfo::verify performs cleanup on the verifier if the verifier throws an exception.  Other callers of Verifier::verify must do the same, but Verifier::verifyFailed does not, when the latter invokes the former with a print writer to print the verification error.  The verification error is thrown twice but because of the simplistic rethrow logic in Verifier::verify only the outermost handler is invoked; it cleans up the outermost Verifier but leaves a Verifier state leaked.

Comment 5

[Lars T Hansen]

Not a security error, declassifying.

Comment 6

[Lars T Hansen]

Attachment: Patch

Simple fix: wrap an exception handler around the recursive verifier call; ignore the error and let end-of-scope cleanup take care of things before we throw the true exception.

Comment 7

[Lars T Hansen]

tamarin-redux-argo changeset:   3932:1869c9b85df1

Comment 8

[Chris Peyer]

Regression tests pushed to redux changeset:   4434:01538155f876

Comment 9

[Chris Peyer]

Still getting the assert on linux-arm beagleboard:

avmshell_neon_arm_d regress/bug_555705.abc_ 

typecheck global/f()
Modified abc throws 'VerifyError: Error #1025: An invalid register 1 was accessed.' PASSED!
Bug 55705 - should not leak memory: PASSED!
Assertion failed: "((leakedBytes == 0 || GetStatus() == kMemAbort))" ("/home/build/buildbot/tamarin-redux/linux/repo/MMgc/GCHeap.cpp":258) Leaks!
Trace/breakpoint trap

Comment 10

[Lars T Hansen]

Is beagleboard-arm-linux still P1 for 10.1?

I'll try to repro on desktop linux, but if I can't then this needs to go by QRB again.

Comment 11

[Lars T Hansen]

Can't repro in debug/nondebugger shell on linux x86 (vmware, ubuntu 9.10).

Requesting QRB reassessment.  If it still needs to be fixed, consider picking a dev who has a working beagleboard setup.

Comment 12

[Dan Smith]

How many bytes are leaked -- is it fixed amount or are does this block the release of the application itself?   If the leakage amount is small then we can consider deferral.  arm-linux is still important for the SDK team.

Comment 13

[Dan Smith]

Assigned to Steven J., please investigate the cause on ARM linux.

Comment 14

[Steven Johnson]

Not reproing on Beagleboard either (using debug/nondebugger shell) -- cpeyer, can you provide any more info?

Comment 15

[Steven Johnson]

Aha -- must be compiled with VFP enabled for the leak to occur, apparently...

Comment 16

[Steven Johnson]

OK, it appears that the issue is that --enable-arm-fpu and --enable-arm-neon both (presumably unintentionally) enable -O3, even in debug builds; removing this makes the leak report go away. However, the likely implication is that -O3 is leaking (but not reporting as such) in non-debug builds, so this smells like a real issue. I'll try to figure out where the leak is occurring...

Comment 17

[Steven Johnson]

(In reply to comment #16)
> OK, it appears that the issue is that --enable-arm-fpu and --enable-arm-neon
> both (presumably unintentionally) enable -O3, even in debug builds

opened https://bugzilla.mozilla.org/show_bug.cgi?id=560699 to correct this issue

Comment 18

[Steven Johnson]

Attachment: add missing volatile

Source of the leak is a missing 'volatile' -- if an exception is thrown, nonvolatile variables might be clobbered. On x86, registers are few, so 'coder' isn't registerized, but apparently on ARM with /O3 coder was in a register, and got reverted to NULL by setjmp/longjmp.

Comment 19

[Lars T Hansen]

Comment on attachment 440363 [details] [diff] [review]
add missing volatile

Are there more variables throughout the VM we should worry about?

Comment 20

[Steven Johnson]

(In reply to comment #19)
> (From update of attachment 440363 [details] [diff] [review])
> Are there more variables throughout the VM we should worry about?

Seems quite possible. I'm rather surprised the compiler didn't give us a "may be clobbered" warning -- maybe we've disabled a warning?

Comment 21

[Lars T Hansen]

Could be the GCC version is too old to support those warnings, too.  But we should have seen it with GCC 4.2 on MacOS, where we don't disable those warnings (we get a few in the interpreter, but they're incorrect).

Wait, that means that we get /bogus/ warnings one place and /no/ warnings where we should have had them...

May also be that GCC's warnings are tricked by our use of _setjmp.

Comment 22

[Steven Johnson]

http://asteam.macromedia.com/hg/tamarin-redux-argo/rev/0e4efa62aeb8

Comment 23

[Chris Peyer]

Testcase reenabled: http://asteam/hg/tamarin-redux-argo/rev/bd1b2086b748

Comment 24

[Chris Peyer]

Actually testcase pushed in this rev (ignore comment 23):

http://asteam/hg/tamarin-redux-argo/rev/9e1807130b28

Comment 25

[Dan Schaffer]

verified the testcase regress/bug_555705.abc_ (debug, -Darmvfp) is passing in tr-argo and tr on arm-linux and other platforms.  marking as verified.