14853 2007-08-01 12:04:02 -0700 Incorrect implementation of ArrayImpl's equality operator 2007-08-29 10:45:11 -0700 1 1 1 Unclassified WebKit WebCore Misc. 312.x All All RESOLVED FIXED P3 Minor --- 1 rick webkit-unassigned oldest_to_newest 3330 0 rick 2007-08-01 12:04:02 -0700 The ArrayImpl class (as defined in WebCore/platform/ArrayImpl.{h,cpp}) has a typo in it's equality operator which could lead to an incorrect equality test and also a buffer overrun (and therefore a potential crash). The fix is a trivial change to one line of code. The erroneous code contains the test: d->itemSize == d->itemSize which should be: d->itemSize == a.d->itemSize In practice, the bug will probably never be seen as the only (current) use of the ArrayImpl class is by DeprecatedArray (and it's subclass DeprecatedCString). This means that the value of itemSize will always be sizeof(char) (i.e. 1) for both objects being compared and the equality test will work correctly. On the other hand, this bug appears in the exact same form in much older versions of ArrayImpl. In particular, the existing code has been copied from WebCore/kwq/KWQArrayImpl.cpp (which was copied from WebCore/kwq/KWQArrayImpl.mm). I didn't dig too deep but previous versions of WebKit may have additional dependencies on these older classes and cases where itemSize is different. This bug may therefore be more serious in older versions of WebKit. 3331 1 15785 rick 2007-08-01 12:06:36 -0700 Created attachment 15785 Patch that fixes the described bug (includes ChangeLog differences) 3157 2 ddkilzer 2007-08-02 06:17:15 -0700 (In reply to comment #1) > Created an attachment (id=15785) [edit] > Patch that fixes the described bug (includes ChangeLog differences) This really needs a test case. 3148 3 rick 2007-08-02 08:04:10 -0700 (In reply to comment #2) > (In reply to comment #1) > > Created an attachment (id=15785) [edit] > > Patch that fixes the described bug (includes ChangeLog differences) > > This really needs a test case. Where can I add such a test case? The problem is that the bug wouldn't show itself with current higher level functionality (i.e. a layout test or a JavaScriptCore test). I couldn't find anything about adding a lower level C++ test program. For example, something like: #include "ArrayImpl.h" int main() { WebCore::ArrayImpl a(1, 1); WebCore::ArrayImpl b(2, 1); // return 1 on error return a == b ? 1 : 0; } 3142 4 ddkilzer 2007-08-02 09:58:08 -0700 (In reply to comment #3) > (In reply to comment #2) > > This really needs a test case. > > Where can I add such a test case? The problem is that the bug wouldn't show > itself with current higher level functionality (i.e. a layout test or a > JavaScriptCore test). I couldn't find anything about adding a lower level C++ > test program. For example, something like: Sorry, by "test case" I meant a layout test (in HTML/JavaScript). Since that's not possible, it's okay not to have one. :) 1244 5 mrowe 2007-08-29 10:45:11 -0700 Landed in r25297. 15785 2007-08-01 12:06:36 -0700 2007-08-02 04:43:45 -0700 Patch that fixes the described bug (includes ChangeLog differences) patch.txt text/plain 1122 rick SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAyNDgwNCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTMgQEAKKzIwMDctMDgtMDEgIHJpY2sgIDxyaWNrQHdyaXRoZS5vcmcudWs+CisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV0FSTklORzog Tk8gVEVTVCBDQVNFUyBBRERFRCBPUiBDSEFOR0VECisKKyAgICAgICAgKiBwbGF0Zm9ybS9BcnJh eUltcGwuY3BwOgorICAgICAgICAoV2ViQ29yZTo6QXJyYXlJbXBsOjpvcGVyYXRvcj09KToKKwlG aXhlZCB0eXBvIHNvIHRoYXQgY29ycmVjdCB2YXJpYWJsZSBpcyB1c2VkIGluIGVxdWFsaXR5IGNv bXBhcmlzb24uCisKIDIwMDctMDctMzEgIERhdmlkIEhhcnJpc29uICA8aGFycmlzb25AYXBwbGUu Y29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEp1c3Rpbi4KSW5kZXg6IFdlYkNvcmUvcGxhdGZv cm0vQXJyYXlJbXBsLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL3BsYXRmb3JtL0FycmF5SW1w bC5jcHAJKHJldmlzaW9uIDI0ODAzKQorKysgV2ViQ29yZS9wbGF0Zm9ybS9BcnJheUltcGwuY3Bw CSh3b3JraW5nIGNvcHkpCkBAIC0xMzgsNyArMTM4LDcgQEAgYm9vbCBBcnJheUltcGw6OmZpbGwo Y29uc3Qgdm9pZCAqaXRlbSwgaQogCiBib29sIEFycmF5SW1wbDo6b3BlcmF0b3I9PShjb25zdCBB cnJheUltcGwgJmEpIGNvbnN0CiB7Ci0gICAgcmV0dXJuIGQtPm51bUl0ZW1zID09IGEuZC0+bnVt SXRlbXMgJiYgZC0+aXRlbVNpemUgPT0gZC0+aXRlbVNpemUKKyAgICByZXR1cm4gZC0+bnVtSXRl bXMgPT0gYS5kLT5udW1JdGVtcyAmJiBkLT5pdGVtU2l6ZSA9PSBhLmQtPml0ZW1TaXplCiAgICAg ICAgICYmIChkLT5kYXRhID09IGEuZC0+ZGF0YSB8fCBtZW1jbXAoZC0+ZGF0YSwgYS5kLT5kYXRh LCBkLT5pdGVtU2l6ZSpkLT5udW1JdGVtcykgPT0gMCk7CiB9CiAK