659989 - Policy overflow with mmgc-demo

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Tommy Reilly]

Attachment: check for int overflow

Encountered this in my convoluted demo code:

hg.mozilla.org/users/treilly_adobe.com/mmgc-demo

spot fix patch attached

Comment 1

[Tommy Reilly]

The failure here is this (maybe new?) assert:

        GCAssert(minorAllocationBudget > 0);

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

(I want to look further into this to understand what scenarios we would end up with such an enormous value for A().  But I'm fine with the patch Tom attached, in part because I was peeking over his shoulder when he wrote it.)

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #2)
> (I want to look further into this to understand what scenarios we would end
> up with such an enormous value for A().  But I'm fine with the patch Tom
> attached, in part because I was peeking over his shoulder when he wrote it.)

On further reflection, I'm not sure we want to do it this way: Do we know that INT_MAX is always 32-bit and not a int64_t?  (Because if it were the max int64_t, then we could fall back into the same problem of making a negative minorAllocationBudget in the int32_t cast.)

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 535369 [details] [diff] [review]
check for int overflow

R- until I'm convinced that INT_MAX is reliably the 32-bit maximum and not the 64-bit one.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

After reading over http://en.wikipedia.org/wiki/Limits.h I'm willing to believe that INT_MAX could be reliable, even though the standard does not dictate that it must be so.

I think I would be satisfied if the patch were extended with:
  STATIC_ASSERT(INT_MAX == int64_t(int32_t(INT_MAX)));

Comment 6

[Steven Johnson]

(In reply to comment #4)
> R- until I'm convinced that INT_MAX is reliably the 32-bit maximum and not
> the 64-bit one.

It's not like the maximum for a 32-bit signed integer is gonna vary...

s/INT_MAX/0x7fffffff/

Comment 7

[Lars T Hansen]

Comment on attachment 535369 [details] [diff] [review]
check for int overflow

Canceling SR until Felix is happy.

Comment 8

[Tommy Reilly]

Attachment: With safety belt

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 538892 [details] [diff] [review]
With safety belt

count me satisfied!

Comment 10

[Tamarin Bot]

changeset: 6428:f73c7c31ab1b
user:      Tommy Reilly <[e-mail]>
summary:   Bug 659989 - Fix policy overflow on double to int conversion, clamp to MAX_INT (r=fklockii,sr=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/f73c7c31ab1b