Bug 70691 – BitVector isInline check could fail

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yuqiang Xian]

Current BitVector uses the highest bit m_bitsOrPointer to indicate whether it's an inlined bit set or a pointer to an outOfLine bit set. This check may fail in case the pointer is also has the highest bit set, which is surely possible on IA32 (Linux). In this case the check failure can result in unexpected behaviors, for example if the BitVector is incorrectly determined as having an inlined bit set, then setting a bit exceeding maxInlineBits will wrongly modify the memory adjacent to the BitVector object, which causes hard-to-rootcause runtime errors, such as the failure of 32bit DFG Release build on Linux w/ the function inlining capability, where in DFGByteCodeParser, the bit set on m_preservedVars wrongly modifies the adjacent memory (&m_parameterSlots).

One possible fix is to use the lowest bit of m_bitsOrPointer to indicate inline or outofline, based on the assumption that the pointer to OutOfLineBits should be 4 or 8 byte aligned (please correct me if we can safely make this assumption). We could mark the lowest bit (bit 0) with 1 for inlined bit set, and bits 1~bitsInPointer are used for bit set/test. In this case we need do one bit more shift for bit set/test.

Patch is forthcoming, while are there any better ideas...?

[Yuqiang Xian]

Created attachment 112108 [details]
proposed patch

[Geoffrey Garen]

> One possible fix is to use the lowest bit of m_bitsOrPointer to indicate inline or outofline, based on the assumption that the pointer to OutOfLineBits should be 4 or 8 byte aligned (please correct me if we can safely make this assumption).

I think this is a reasonable assumption.

You can do two things to make your assumption clearer to the reader:

(1) Add an ASSERT(!isInline()) at the end of BitVector::resizeOutOfLine()

(2) Add a comment above the declaration of m_bitsOrPointer explaining what the low bit means:

"The low bit of m_bitsOrPointer is a flag indicating whether this field is inline bits or a pointer to out of line bits. If the flag is set, the field is inline bits. This works because the low bit in a pointer is always unset."

[Geoffrey Garen]

Comment on attachment 112108 [details]
proposed patch

The approach here is good, but I think a few tweaks would make this patch even better.

[Yuqiang Xian]

Created attachment 112133 [details]
the patch

Patch addressing the review comments. Thanks.

[Geoffrey Garen]

Comment on attachment 112133 [details]
the patch

r=me

[WebKit Review Bot]

Comment on attachment 112133 [details]
the patch

Clearing flags on attachment: 112133

Committed r98268: <http://trac.webkit.org/changeset/98268>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

This fix was actually wrong in two ways:

1) It adds one to the index in the bitops only in the mask part, so for example accessing bit 31 on 32-bit systems will instead access bit 0 in the first word.

2) It does not account for the "lost bit" in the out-of-line bit storage.

I think that the simpler fix is to:

1) Switch back to the way we did things before, since fixing both (1) and (2) seems like begging for off-by-one errors.

2) Fix the original bug by storing the out-of-line bits pointer right-shifted by 1, to make room for the is-inline marker in the top bit.

[Filip Pizlo]

Created attachment 117365 [details]
the patch

[Filip Pizlo]

<rdar://problem/10509659>

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/101639

[Filip Pizlo]

Comment on attachment 117365 [details]
the patch

Clearing flags.

[Yuqiang Xian]

Oops... sorry for introducing the bug...