Closed
Bug 604062
Opened 14 years ago
Closed 14 years ago
AvmAssert(atom == coerce(atom, t)) can fail with false positive
Categories
(Tamarin Graveyard :: Virtual Machine, defect, P1)
Tamarin Graveyard
Virtual Machine
Tracking
(Not tracked)
RESOLVED
FIXED
flash10.2.x-Spicy
People
(Reporter: edwsmith, Assigned: edwsmith)
References
Details
Attachments
(1 file, 1 obsolete file)
6.93 KB,
patch
|
wmaddox
:
review+
|
Details | Diff | Splinter Review |
in BaseExecMgr::unbox1(), we do this: AvmAssert(atom == env->toplevel()->coerce(atom, t)); // Atom must be correct To ensure the atom has been coerced to the proper type before we get here. When t is int or uint, and the incoming atom is kDoubleType (maybe a large, but legal, int value), then coerce() can allocate a new atom, and the == test can fail. The check needs to be refactored to avoid this false failure. It probably should use istype().
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → edwsmith
Priority: -- → P1
Target Milestone: --- → flash10.2.x-Spicy
Comment 1•14 years ago
|
||
Moved to Serrano, as this only happens in OSR context.
Flags: flashplayer-qrb?
Target Milestone: flash10.2.x-Spicy → flash10.x - Serrano
Assignee | ||
Updated•14 years ago
|
Target Milestone: flash10.x - Serrano → flash10.2.x-Spicy
Assignee | ||
Comment 2•14 years ago
|
||
Since unbox1() is only ever called on a receiver object that is not null, istype() can be used to assert the correct type without inducing any value conversions or toString()/valueOf() callbacks.
Attachment #490670 -
Flags: review?(wmaddox)
Updated•14 years ago
|
Attachment #490670 -
Flags: review?(wmaddox) → review+
Assignee | ||
Comment 3•14 years ago
|
||
Comment on attachment 490670 [details] [diff] [review] call istype() instead of coerce() Bah, this is wrong. unbox1() still fails when called during OSR transition, which was the new use case that triggered this bug in the first place. To be correct, it needs to handle null/undefined atoms after all. And, to be even more precise, we should validate using everything we know in FrameState.value[i], namely: Traits*, not_null, and SlotStorageType. new patch coming soon.
Attachment #490670 -
Attachment is obsolete: true
Comment 4•14 years ago
|
||
(In reply to comment #2) > Created attachment 490670 [details] [diff] [review] > call istype() instead of coerce() > > Since unbox1() is only ever called on a receiver object that is not null, > istype() can be used to assert the correct type without inducing any value > conversions or toString()/valueOf() callbacks. I should have done a test build rather than just eyeballing the code. Applying the patch, I get the following build error. It looks like some changes were missing from the patch. Did you forget to 'hg add' exec-osr.cpp before 'hg diff'? /Users/wmaddox/tr-spicy-osr/core/exec-osr.cpp: In static member function ‘static void avmplus::OSR::unboxSlot(avmplus::BaseExecMgr*, avmplus::FrameState*, avmplus::MethodEnv*, avmplus::Atom*, avmplus::FramePtr_*, uint8_t*, int)’: /Users/wmaddox/tr-spicy-osr/core/exec-osr.cpp:289: error: no matching function for call to ‘avmplus::BaseExecMgr::unbox1(avmplus::MethodEnv*&, avmplus::Atom&, avmplus::Traits*&, avmplus::Atom*&)’ /Users/wmaddox/tr-spicy-osr/core/exec.h:223: note: candidates are: static avmplus::Atom* avmplus::BaseExecMgr::unbox1(avmplus::Atom, avmplus::Traits*, avmplus::Atom*) make: *** [core/exec-osr.o] Error 1 wmaddox-macpro:rel32 wmaddox$
Assignee | ||
Comment 5•14 years ago
|
||
The patch I uploaded was for tr-spicy. OSR depends on the fix, not the other way around. My intent is to land the fix in tr-spicy, then tr-osr-spicy can merge from there. In the OSR code, there is one additional call to unbox1() and since I removed the MethodEnv* parameter, gcc is complaining.
Comment 6•14 years ago
|
||
(In reply to comment #5) > The patch I uploaded was for tr-spicy. OSR depends on the fix, not the other > way around. My intent is to land the fix in tr-spicy, then tr-osr-spicy can > merge from there. Ah, sorry. I didn't catch that. Indeed, it's not OSR-specific.
Assignee | ||
Comment 7•14 years ago
|
||
This patch applies to tr-spicy-osr.new. Since OSR code calls unbox1() in a new way, it makes sense to fix it all in the OSR branch. 1. Loosened the assert in unbox1() to handle null/undefined as well. 2. Added a tighter check called from OSR::unboxSlot() to validate all of the invariants in FrameState.Value (type, notNull, and sst_mask). Patch contains some cleanup: * Removed "exec" parameter to unboxSlot(). * Removed "env" paramter to unbox1().
Attachment #490862 -
Flags: review?(wmaddox)
Updated•14 years ago
|
Attachment #490862 -
Flags: review?(wmaddox) → review+
Comment 8•14 years ago
|
||
http://asteam.macromedia.com/hg/users/wmaddox/tr-spicy-osr.new/rev/11515b21745d
Comment 9•14 years ago
|
||
This fix will migrate into TR when we import OSR. Closing.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Flags: flashplayer-qrb? → flashplayer-qrb+
You need to log in
before you can comment on or make changes to this bug.
Description
•