519283 - FixedMalloc should not return NULL when the requested object is too large

Status: VERIFIED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Lars T Hansen]

... unless kCanFail is specified, that is.

Instead, FixedMalloc should call GCHeap::SignalObjectSizeOverflow, which is being introduced as part of the fix in bug #517856.

Must also check the code in GC::Alloc, GCLargeAlloc::Alloc, and maybe others.

Comment 1

[Lars T Hansen]

So far, excluding the above mentioned bug:

FixedMalloc::Alloc
FixedMalloc::Calloc
GC::Alloc

Comment 2

[Lars T Hansen]

GC::Calloc

Comment 3

[Lars T Hansen]

More generally, a number of MMgc APIs return NULL when they should not, when various error conditions occur:

GC::Alloc returns NULL in DEBUG mode if MMGC_GCENTER has not been called.

GC::FindBeginningGuarded returns NULL if the address is not managed.

FixedAlloc::Alloc returns NULL if CreateChunk failed to fill the free list and did not abort.

GCHeap::Alloc returns NULL if AllocBlock failed to expand the heap and did not abort.

Comment 4

[Lars T Hansen]

Attachment: Patch for all the issues

Comment 5

[Lars T Hansen]

Comment on attachment 403740 [details] [diff] [review]
Patch for all the issues

The patch is straightforward and includes the SignalObjectTooLarge patch from bug #517856.

The main issue is that I've introduced two methods that unceremoniously call VMPI_exit when they are called (after asserting and printing some debug info).  This is an improvement on the old return-NULL behavior, and as an abstraction it's probably OK; the question is whether these methods should be doing something more sophisticated, like calling the GC's Abort() method as that will result in a clean shutdown of the host code (which can be a plugin).  IMO we can land this patch (with your approval...) before settling that issue, but we need to settle it.

Comment 6

[Lars T Hansen]

Looping in Rishit for a discussion of what the best thing to do is when we hit an implementation limit.

Comment 7

[Lars T Hansen]

One case I missed before:

GCObject::operator new

Comment 8

[Lars T Hansen]

Comment on attachment 403740 [details] [diff] [review]
Patch for all the issues

Another patch is going to be arriving first and will make this one largely obsolete; will resubmit if necessary.

Comment 9

[Lars T Hansen]

Attachment: Revised patch

This patch is a proper subset of the previous patch; it will land after bug #519980 has closed.

Comment 10

[Lars T Hansen]

redux changeset:   2664:90b3d1f29e29

I made SignalObjectTooLarge and SignalHeapStateInconsistent call GCHeap::Abort instead of VMPI_exit, as that seems most reasonable.

Comment 11

[Brent Baker]

QE: We can add a couple of simple checks for this into the extensions/ST_mmgc_basic.st