Closed
Bug 596556
Opened 14 years ago
Closed 6 years ago
Crack down on VMPI_alloca abuse
Categories
(Tamarin Graveyard :: Virtual Machine, defect)
Tamarin Graveyard
Virtual Machine
Tracking
(Not tracked)
RESOLVED
WONTFIX
Future
People
(Reporter: lhansen, Unassigned)
References
Details
Attachments
(1 file)
995 bytes,
patch
|
edwsmith
:
review+
|
Details | Diff | Splinter Review |
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•14 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•14 years ago
|
||
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•14 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);
Comment 4•14 years ago
|
||
Related bug: https://bugzilla.mozilla.org/show_bug.cgi?id=551766
Reporter | ||
Comment 5•14 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: → 551766
Reporter | ||
Updated•14 years ago
|
Attachment #475476 -
Flags: review?(treilly) → review?(edwsmith)
Comment 6•14 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•14 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•14 years ago
|
Whiteboard: has-patch
Reporter | ||
Comment 8•14 years ago
|
||
Comment on attachment 475476 [details] [diff] [review]
Fix for FileClass
tamarin-redux changeset: 5228:646b4ff57973 (with requested changes)
Reporter | ||
Comment 9•14 years ago
|
||
(Leaving this open to track the work item, I may do more here.)
Whiteboard: has-patch
Reporter | ||
Updated•14 years ago
|
Reporter | ||
Updated•14 years ago
|
Reporter | ||
Updated•14 years ago
|
Assignee: lhansen → nobody
Status: ASSIGNED → NEW
Updated•14 years ago
|
Flags: flashplayer-bug-
Reporter | ||
Comment 10•14 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•14 years ago
|
Priority: P3 → --
Target Milestone: flash10.x-Serrano → Future
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•