Crack down on VMPI_alloca abuse

NEW
Unassigned

Status

Tamarin
Virtual Machine
8 years ago
7 years ago

People

(Reporter: Lars T Hansen, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
Future
Bug Flags:
flashplayer-qrb +
flashplayer-bug -

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
VMPI_alloca is problematic for several reasons.

One, the memory it allocates is treated as pointer-containing (it's on the stack, or in a root, depending on platform and how large the object is), yet for the most part the data stored in that memory is not pointer-containing.  There's a significant risk of misidentification.

Two, conservative scanning of a root is more expensive than no scanning (duh).

Three, pinning of stack memory is /really/ expensive, and from the stack we even retain objects that are reached by interior pointers, further increasing scanning expense and false retention rates.

Four, for large alloca requests we create root memory and this means creating root segments for it, which adds to the set of roots and basically increases the root scanning burden (all roots are scanned twice).

Though it may not be substantially cheaper to allocate non-pointer-containing temp memory on the heap which is then discarded pretty quickly, there are significant indirect benefits from doing so.  We should use VMPI_alloca more sparingly than now.
(Reporter)

Comment 1

8 years ago
Another tweak that might be useful is to introduce a variant of VMPI_alloca that explicitly allocates non-pointer-containing memory.
(Reporter)

Comment 2

8 years ago
Created attachment 475476 [details] [diff] [review]
Fix for FileClass

FileClass is shell code and not a big deal, but the ESC test is affected by this: a buffer the size of the file is allocated by VMPI_alloca.
Attachment #475476 - Flags: review?(treilly)
(Reporter)

Comment 3

8 years ago
Candidates (perhaps without obvious fixes, and perhaps not obviously wrong):

AvmCore.cpp:
 wchar *buffer = (wchar*) VMPI_alloca(this, _buffer, (len16+1)*sizeof(wchar));
 wchar* swapped = (wchar*)VMPI_alloca(this, _swapped, sizeof(wchar)*(len));

MathUtils.cpp:
 char* buffer = (char*)VMPI_alloca(core, _buffer, kMinSizeForInt64_t_toString);
 char* tmp = (char*)VMPI_alloca(core, _tmp, kMinSizeForDouble_base2_toString);
 char* buffer = (char*)VMPI_alloca(core, _buffer,
                                   kMinSizeForDouble_base10_toString);

eval-cogen-stmt.cpp:
 Lcase = (Label**)VMPI_alloca(compiler->context->core,
                              _Lcase, sizeof(Label*) * ncases);
(Reporter)

Comment 5

8 years ago
You're right - they're more or less the same.  Wonder why my search did not find it?  I'll consolidate / divide up tasks.
See Also: → bug 551766
(Reporter)

Updated

8 years ago
Attachment #475476 - Flags: review?(treilly) → review?(edwsmith)

Comment 6

8 years ago
Comment on attachment 475476 [details] [diff] [review]
Fix for FileClass

If a valid BOM is found, the string is created and returned without calling gc->Free().

If the newString*() methods are guaranteed not to throw, then a fixedmem allocation could be used.  just observing, not quite suggesting.

R+ with those points considered, take action or not as you see fit.
Attachment #475476 - Flags: review?(edwsmith) → review+
(Reporter)

Comment 7

8 years ago
(In reply to comment #6)
> Comment on attachment 475476 [details] [diff] [review]
> Fix for FileClass
> 
> If a valid BOM is found, the string is created and returned without calling
> gc->Free().

OK, willfix.

> If the newString*() methods are guaranteed not to throw, then a fixedmem
> allocation could be used.  just observing, not quite suggesting.

Pointer-free GC allocations are not, as a rule, more expensive than fixed allocations, and are properly cleaned up to boot in case of exceptions.  So I'll probably leave that alone.
(Reporter)

Updated

8 years ago
Whiteboard: has-patch
(Reporter)

Comment 8

8 years ago
Comment on attachment 475476 [details] [diff] [review]
Fix for FileClass

tamarin-redux changeset:   5228:646b4ff57973 (with requested changes)
(Reporter)

Comment 9

8 years ago
(Leaving this open to track the work item, I may do more here.)
Whiteboard: has-patch
(Reporter)

Updated

8 years ago
Blocks: 599815
(Reporter)

Updated

8 years ago
No longer blocks: 576307
Depends on: 576307
(Reporter)

Updated

8 years ago
Blocks: 576307
No longer depends on: 576307
(Reporter)

Updated

8 years ago
No longer blocks: 576307
(Reporter)

Updated

8 years ago
Assignee: lhansen → nobody
Status: ASSIGNED → NEW

Updated

8 years ago
Flags: flashplayer-bug-
(Reporter)

Comment 10

7 years ago
So far as I can tell, all uses of VMPI_alloca in the Flash Player are for allocating space for avmplus::Atom arrays (for arguments to a call), so a non-pointer API would only benefit Tamarin.
(Reporter)

Updated

7 years ago
Priority: P3 → --
Target Milestone: flash10.x-Serrano → Future

Updated

7 years ago
Flags: flashplayer-qrb+
You need to log in before you can comment on or make changes to this bug.