600687 - ArenaBitmap does not account for color

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

ArenaBitmap contains:

    JS_ALWAYS_INLINE void mark(size_t bit, uint32 color) {
        uintptr_t *word = &bitmap[bit / BitsPerWord];
        JS_ASSERT(word < &bitmap[JS_ARRAY_LENGTH(bitmap)]);
        *word |= (uintptr_t(1) << ((bit + color) % BitsPerWord));
    }

Here the word calculation does not add color to the bit leading to the wrong word selected when bit + color divides BitsPerWord effectively treating the pair (bit, color) as if it is (bit + color - BitsPerWord, 0). 

In practice, given that the code always use the above pattern, the bug can lead to bad consequences only if the number of cells in GC thing divides BitsPerWord - 1. Only then it is possible that the code maps black bit into used white one. On 32 bit CPU that means only GC things with 31 cells will be affected and the bug is not possible. On 64 bits, given that 63 = 3*3*7, more bad sizes are possible but none of them matches the size of object, function or xml (13, 18 or 20 GC cells).

So we are safe until the bug 584917 lands.

Comment 1

[Gregor Wagner [:gwagner]]

This function is not used at all if I remember correctly. I wanted to remove it but it somehow stayed in. Lets just remove it?

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> This function is not used at all if I remember correctly. I wanted to remove it
> but it somehow stayed in. Lets just remove it?

All 3 functions has this bug. But if mark is not used, then I will remove it.

Comment 3

[Igor Bukanov]

Attachment: v1

Cell::mark was not used indeed. For consistency with the rest of code the patch replaces the custom BitsPerWord constant with the usage of JS_BITS_PER_WORD.

Comment 4

[Gregor Wagner [:gwagner]]

Comment on attachment 479581 [details] [diff] [review]
v1

thx!

Comment 5

[Igor Bukanov]

Nominating for 2.0 to have this regression fixed to avoid bad suprises if the structure of JSObject changes or if the bug 584917 would land there.

Comment 6

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/def89312ea4f

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/def89312ea4f