522803 - AvmCore::isInteger, Avm::integer aren't 64-bit safe, sorta

Status: VERIFIED DUPLICATE of bug 523192

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

On 32-bit builds, an Atom of type kIntegerType can hold a signed int28, but on 64-bit builds, can hold a signed or unsigned int32. The problem is that code of the form

    if (AvmCore::isInteger(atom))
       int32_t i = AvmCore::integer(atom);

exists in embedder code (and probably in tamarin itself, I haven't checked) but it subtly wrong for 64-bit builds, since "isInteger" can return true for values > 0x7fffffff, causing sign flips for unsigned values.

Not sure of the best fix; changing Atom representation isn't a good idea for various reasons at the moment. Perhaps changing the access API to have "isInt32", "isUint32", etc would improve things.

Comment 1

[Steven Johnson]

Attachment: Patch

The crux of the problem is that while both AvmCore::isInteger and AvmCore::integer do what they promise, they tend to be misused because it works fine on 32-bit systems. In particular, isInteger means "are you an integer atom" but it's often used in the sense of "will you fit into an int32", which is always true for 32-bit but not for 64-bit.

This patch addresses that by removing AvmCore::isInteger entirely, and replaces it with atomIsInt32, atomIsUint32, and atomIsPositiveInt (see atom-inlines.h for details), the theory being that these are much more descriptive and less likely to be misused by the caller. (For the rare cases where we really do need to know "is atomKind kIntegerType?" that's what we check now.)

Note that AvmCore::integer didn't get any attention because it isn't wrong.

It also replaces the misleadingly named atomInt() with atomIntptr(), atomInt32(), and atomUInt32(). atomIntptr() is identical to atomInt() [it always returned an intptr], and the other two handle the common cases of immediately casting the result of atomIntptr() to int32 or uint32.


The rest of the patch is fixing callers to the above to be correct. (Note that I didn't find any that were obviously wrong; the genesis of this patch is in embedder code, which was using isInteger incorrectly.)

Comment 2

[Lars T Hansen]

Nomenclature nit apropos atomIsPositiveInt(): "positive" usually means "greater than zero", while "nonnegative" is often used to denote "greater than or equal to zero".  Since the method checks for greater than or equal to zero, atomIsNonnegativeInt might be less ambiguous.

Comment 3

[Edwin Smith]

see also: bug 521353 reports on problems manipulating large kIntegerType atoms.

Comment 4

[Steven Johnson]

(In reply to comment #2)
> Nomenclature nit apropos atomIsPositiveInt()

Point taken, but "atomIsNonNegativeInt" seems wordy and "positive" is IMHO not too misleading here, but I can be persuaded.

Comment 5

[Steven Johnson]

(In reply to comment #4)
> (In reply to comment #2)
> > Nomenclature nit apropos atomIsPositiveInt()
> 
> Point taken, but "atomIsNonNegativeInt" seems wordy and "positive" is IMHO not
> too misleading here, but I can be persuaded.

On the other hand, if accuracy isn't important, we can make it as terse as we like :-)

Comment 6

[Edwin Smith]

Comment on attachment 406822 [details] [diff] [review]
Patch

AvmCore.cpp:1946 in increment_d, the code is not 64bit safe.   basically you can't ever explicitly cast (atom>>3) to int32_t unless you're about to store it in a :int typed slot or pass it to an int typed parameter.  

(and, maybe s/sint32/int32_t while we're at it)

AvmCore.cpp:3745  this explicit cast is okay since integer() is defined to return int32_t values even if they start out larger.

CodegenLIR.cpp:661 same problem, not 64bit safe to assume that kIntegerType implies int32_t

Comment 7

[Steven Johnson]

(In reply to comment #6)
> AvmCore.cpp:1946 in increment_d, the code is not 64bit safe.   basically you
> can't ever explicitly cast (atom>>3) to int32_t unless you're about to store
> it in a :int typed slot or pass it to an int typed parameter.  

Actually, you *can* if you have just called atomIsInt32()... that's the point of the call, in fact. (If atomIsInt32 doesn't guarantee that then it needs work...)

> (and, maybe s/sint32/int32_t while we're at it)

agreed. Even better, that line should just be

    *ap = intToAtom(atomInt32(a)+delta);
 
> AvmCore.cpp:3745  this explicit cast is okay since integer() is defined to
> return int32_t values even if they start out larger.

Um, right, so this is a non-problem?

> CodegenLIR.cpp:661 same problem, not 64bit safe to assume that kIntegerType
> implies int32_t

Doh. Will fix and resubmit.

Comment 8

[Steven Johnson]

Attachment: Patch #2

Revised version that addresses (I think) the concerns you have.

Comment 9

[Edwin Smith]

increment_d is supposed to just add 1 to whatever number is passed in.  on 64bit, you'll get kIntegerType atoms that are >32 bits.  So, the proper test is isInteger(*ap), and the addition logic should do all its math with intptr_t.

You're right that atomInt32() is "safe" if you've just called atomIsInt32(), however code like this is unsafe:

   if (atomIsInt32(a)) { ... }
   else {
      .. atomToDouble(a)
   }

because in the else case, you'll still have kIntegerType atoms that are not in the 32bit range, and the above code will try to dereference them as if they were kDoubleType pointers.

If this all happens in a context that is supposed to truncate to int32, then you want the test to be isInteger() (to catch *all* the kIntegerType values), and then extract & truncate to int32_t using atomInt32(). (or uint32_t).  but if the context is generally Number, as in increment_d(), then we want isInteger() and all operations using intptr_t.

32-bit safe code can't assume that int32 values are kIntegerType (sometimes they are kDoubleType), and 64-bit safe code cannot assume that kIntegerType values are int32 values.  fun, huh?

Comment 10

[Edwin Smith]

Attachment: number_d incorrectly truncates kIntegerType to 32 bits

the proper conversion is kIntegerType -> intptr_t -> double

Comment 11

[Steven Johnson]

(In reply to comment #9)

> 32-bit safe code can't assume that int32 values are kIntegerType (sometimes
> they are kDoubleType), and 64-bit safe code cannot assume that kIntegerType
> values are int32 values.  fun, huh?

Yuck. Sounds like solving this really means we need to nail down https://bugzilla.mozilla.org/show_bug.cgi?id=521353 first. 

Further, this bug probably is better off closed out and merged in with https://bugzilla.mozilla.org/show_bug.cgi?id=523192 -- the real question here is if there is an atom-access-api that makes it easy to do the right thing and hard (or impossible) to do the wrong thing.

Comment 12

[Steven Johnson]

Thinking about this a little further... what we really want is an API that shields the caller from the internal rep of the Atom. The interesting questions are:

-- are you a numeric type?
-- can I extract your value as an int32 / uint32 / double? (perhaps also intptr)

but the most crucial thing is to sever the expectation that "integer atom" implies "fits in 32 bits", since 64-bit will have larger (probably 53-bit) precision.

If/when you land your increment_d patch, let's close this bug and move the discussion to 523192.

Comment 13

[Edwin Smith]

that other patch was for number_d, but good idea, i can make a new one to fix the other cases, to separate it from the api cleanup.

Comment 14

[Edwin Smith]

Comment on attachment 407282 [details] [diff] [review]
number_d incorrectly truncates kIntegerType to 32 bits

pushed http://hg.mozilla.org/tamarin-redux/rev/81e3ab776f5f

Comment 15

[Edwin Smith]

closing this out as dupe