With no heap limit a test that just appends a string to itself over and over crashes, should exit with OOM exit code.

(poaching)

Obvious test case. Provokes the described behavior.

In debug builds, String::_append computes a large negative length and then String::createDynamic barfs immediately on entry because the length is invalid. In release builds, pretty much the same thing happens; then this code in createDynamic kicks in: int32_t alloc = len + extra; if (alloc < 1) alloc = 1; Ouch. The allocation succeeds, after which 200MB of string data are copied into a 32-byte buffer.

(In reply to comment #3) > 200MB of string data are copied into a 32-byte buffer. That must be some mighty effective data-compression...

Generally speaking we need to be more careful about dealing with object size overflow. I suspect the custom differs from place to place in the VM. The memory allocators have been returning NULL when the requested size exceeds the maximum object size; this is pretty nutty and I have a couple of bugs on fixing that (bug #517856, bug #519283), but at least they're not ignoring the problem like the string code does in the present case. The method I've adopted in the allocators is to pass the problem on to a common hook in GCHeap which can take some structured abort action a la what we use for OOM; clean shutdown is what we want. (The current implementation of that hook just calls VMPI_exit(1), which is not right but no worse than what it was.) I'm guessing that what we want is a clearer notion of implementation limits in the VM and what to do when they are exceeded, in cases not currently covered.

There are two implied invariants of the string code: - the length always fits in an int32_t - the number of bytes required to represent the string always fits in an int32_t. (This latter invariant becomes interesting for characters that don't fit in 8 bits.) This patch adds checking for those invariants and computations that guard against overflow when string lengths and buffer sizes are computed. Overflows result in termination of the program through a call to GCHeap::SignalObjectTooLarge. The hard part is to know that all paths through the code have been covered. I've only examined the string code proper; there may be problems with code calling the string code that I've not found. There's a small amount of drive-by cleanup, all related to this patch.

rat-ta-tat-tat, driveby cleanup is a good thing.

Comment on attachment 405459 [details] [diff] [review] Add compile-time and run-time checking for length overflow redux changeset: 2748:fbe1b2a374f6

Trevor, can you make sure the test case gets integrated into our regression testing somehow? It should be valid on both 32-bit and 64-bit systems. The desired result is a clean OOM abort.

If a bug requires a testcase to be added please set the "in‑testsuite" flag to "?" and QE will pick it up.

It is valid to run the testcase with a -memlimit instead of waiting for the machine to run out of memory correct?

No, we need to go as far as we can to get the 32 bit math to go wonky.

(In reply to comment #12) > No, we need to go as far as we can to get the 32 bit math to go wonky. Correct.

Is it a bug that windows takes ~2min to hit the OOM (avm release) whereas osx takes ~10s. Also - release debugger asserts with: osx: Assertion failed: "(((!"Too-large object request; aborting")))" ("/Users/build/buildbot/tamarin-redux/mac-intel-10_5/tamarin-redux/MMgc/GCHeap.cpp":1687) Abort trap win: Assertion failed: "(((!"Too-large object request; aborting")))" ("c:/buildbot/tamarin-redux/windows/tamarin-redux/MMgc/GCHeap.cp avmplus crash: exception 0x80000003 occurred Writing minidump crash log to avmplusCrash.dmp

After the assert, the vm exits with exitcode 134. Release player exits correctly with exitcode 128: Implementation limit exceeded: attempting to allocate too-large object error: out of memory

In retrospect adding the GCAssert was the wrong thing, and that's the problem here. Should be easy to fix.

Removed that assert, pushed the fix.

(In reply to comment #17) > Removed that assert, pushed the fix. redux changeset: 2810:c30a191d00a7

Verified fixed mac & win redux 2817. Test media pushed redux 2819:bbfee868f052

Still failing on win64: unexpected exit code expected:128 actual:3221226505 FAILED!

The Win64 problem is probably caused by bug #497669: we are using the Microsoft versions of setjmp/longjmp, which unwind the stack. We can't do that, we must use controlled versions that don't unwind but just jump. (This is based on running the test in a debugger and getting run-time errors inside RtlUnwindEx, in which we should never be finding ourselves.)

win64 problem fixed by fix to bug #497669: redux changeset: 2881:b80e5c6070d6

verified win64 fixed in build 2894