|Summary:||BitVector isInline check could fail|
|Product:||WebKit||Reporter:||Yuqiang Xian <yuqiang.xian>|
|Severity:||Normal||CC:||barraclough, fpizlo, ggaren, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Yuqiang Xian 2011-10-23 05:16:56 PDT
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...?
Comment 2 Geoffrey Garen 2011-10-23 11:33:08 PDT
> 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."
Comment 3 Geoffrey Garen 2011-10-23 11:34:30 PDT
Comment on attachment 112108 [details] proposed patch The approach here is good, but I think a few tweaks would make this patch even better.
Comment 4 Yuqiang Xian 2011-10-23 18:09:11 PDT
Created attachment 112133 [details] the patch Patch addressing the review comments. Thanks.
Comment 5 Geoffrey Garen 2011-10-24 12:07:14 PDT
Comment on attachment 112133 [details] the patch r=me
Comment 6 WebKit Review Bot 2011-10-24 12:40:39 PDT
Comment on attachment 112133 [details] the patch Clearing flags on attachment: 112133 Committed r98268: <http://trac.webkit.org/changeset/98268>
Comment 7 WebKit Review Bot 2011-10-24 12:40:43 PDT
All reviewed patches have been landed. Closing bug.
Comment 8 Filip Pizlo 2011-12-01 00:12:21 PST
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.
Comment 12 Filip Pizlo 2011-12-01 01:38:19 PST
Comment on attachment 117365 [details] the patch Clearing flags.
Comment 13 Yuqiang Xian 2011-12-01 04:02:57 PST
Oops... sorry for introducing the bug...