Closed
Bug 515935
Opened 15 years ago
Closed 15 years ago
String doubling test crashes because length computation goes negative
Categories
(Tamarin Graveyard :: Virtual Machine, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
flash10.1
People
(Reporter: treilly, Assigned: lhansen)
References
Details
(Whiteboard: Needs TC)
Attachments
(2 files, 1 obsolete file)
With no heap limit a test that just appends a string to itself over and over crashes, should exit with OOM exit code.
Updated•15 years ago
|
Flags: flashplayer-triage+
Flags: flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.1
Assignee | ||
Comment 2•15 years ago
|
||
Obvious test case. Provokes the described behavior.
Assignee | ||
Comment 3•15 years ago
|
||
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.
Assignee: lhansen → nobody
Summary: String doubling test crashes → String doubling test crashes because length computation goes negative
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → daumling
Comment 4•15 years ago
|
||
(In reply to comment #3)
> 200MB of string data are copied into a 32-byte buffer.
That must be some mighty effective data-compression...
Assignee | ||
Comment 5•15 years ago
|
||
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.
Component: Garbage Collection (mmGC) → Virtual Machine
QA Contact: gc → vm
Assignee | ||
Updated•15 years ago
|
Assignee: daumling → nobody
Priority: P3 → P2
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → lhansen
Assignee | ||
Comment 6•15 years ago
|
||
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.
Attachment #405459 -
Flags: review?(edwsmith)
Updated•15 years ago
|
Attachment #405459 -
Flags: review?(edwsmith) → review+
Comment 7•15 years ago
|
||
rat-ta-tat-tat, driveby cleanup is a good thing.
Assignee | ||
Comment 8•15 years ago
|
||
Comment on attachment 405459 [details] [diff] [review]
Add compile-time and run-time checking for length overflow
redux changeset: 2748:fbe1b2a374f6
Attachment #405459 -
Attachment is obsolete: true
Assignee | ||
Comment 9•15 years ago
|
||
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.
Assignee: lhansen → trbaker
Whiteboard: Needs TC
Updated•15 years ago
|
Flags: in-testsuite?
Comment 10•15 years ago
|
||
If a bug requires a testcase to be added please set the "in‑testsuite" flag to "?" and QE will pick it up.
Comment 11•15 years ago
|
||
It is valid to run the testcase with a -memlimit instead of waiting for the machine to run out of memory correct?
Reporter | ||
Comment 12•15 years ago
|
||
No, we need to go as far as we can to get the 32 bit math to go wonky.
Assignee | ||
Comment 13•15 years ago
|
||
(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.
Comment 14•15 years ago
|
||
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
Comment 15•15 years ago
|
||
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
Updated•15 years ago
|
Assignee: trbaker → lhansen
Assignee | ||
Comment 16•15 years ago
|
||
In retrospect adding the GCAssert was the wrong thing, and that's the problem here. Should be easy to fix.
Assignee | ||
Comment 17•15 years ago
|
||
Removed that assert, pushed the fix.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 18•15 years ago
|
||
(In reply to comment #17)
> Removed that assert, pushed the fix.
redux changeset: 2810:c30a191d00a7
Comment 19•15 years ago
|
||
Verified fixed mac & win redux 2817. Test media pushed redux 2819:bbfee868f052
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Updated•15 years ago
|
Flags: in-testsuite+
Comment 20•15 years ago
|
||
Still failing on win64:
unexpected exit code expected:128 actual:3221226505 FAILED!
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 21•15 years ago
|
||
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.)
Depends on: 497669
Assignee | ||
Comment 22•15 years ago
|
||
win64 problem fixed by fix to bug #497669: redux changeset: 2881:b80e5c6070d6
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•