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)

Tracking

(Not tracked)

RESOLVED FIXED
flash10.2.x-Spicy

People

(Reporter: edwsmith, Assigned: edwsmith)

References

Details

Attachments

(1 file, 1 obsolete file)

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().
Blocks: OSR
Severity: normal → minor
Assignee: nobody → edwsmith
Priority: -- → P1
Target Milestone: --- → flash10.2.x-Spicy
Moved to Serrano, as this only happens in OSR context.
Flags: flashplayer-qrb?
Target Milestone: flash10.2.x-Spicy → flash10.x - Serrano
Target Milestone: flash10.x - Serrano → flash10.2.x-Spicy
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)
Attachment #490670 - Flags: review?(wmaddox) → review+
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
(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$
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.
(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.
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)
Attachment #490862 - Flags: review?(wmaddox) → review+
This fix will migrate into TR when we import OSR.  Closing.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: flashplayer-qrb? → flashplayer-qrb+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: