120802 2013-09-05 15:22:05 -0700 [CSS Shapes] Heap-buffer-overflow in WebCore::ShapeInterval<float>::subtractShapeIntervals 2013-09-13 10:26:17 -0700 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 giles_joplin giles_joplin commit-queue esprehn+autocc glenn kondapallykalyan oldest_to_newest 925244 0 giles_joplin 2013-09-05 15:22:05 -0700 The attached test caused a crash when run with a bounds checker, see https://code.google.com/p/chromium/issues/detail?id=285788. The source of the problem are several assignments like this: static void subtractShapeIntervals(const ShapeIntervals& a, const ShapeIntervals& b, ShapeIntervals& result) { ... const_iterator aNext = a.begin(); const_iterator bNext = b.begin(); ShapeInterval<T> aValue = *aNext; ShapeInterval<T> bValue = *bNext; while (aNext != a.end() && bNext != b.end()) { if (bValue.contains(aValue)) aValue = *++aNext; // BAD ... } .... } Although the LHS of the assignments aren't used when the corresponding iterator has reached its end, the expression still dereferences an invalid address (a.end() in this case). The loop has been rewritten; we do not dereference the aNext or bNext iterators without checking the end condition. 925245 1 210670 giles_joplin 2013-09-05 15:22:50 -0700 Created attachment 210670 Test case. 925254 2 210673 giles_joplin 2013-09-05 15:43:23 -0700 Created attachment 210673 Patch 925836 3 210673 darin 2013-09-06 13:53:44 -0700 Comment on attachment 210673 Patch The coding style here is awkward. The names “a/b increment” are not so good. I would call them shouldIncrementA/B instead. Also, the logic seems unnecessarily twisted. I have to read it over and over again to be sure it’s right. Might be worth another look to see if we can make the logic clearer. I think we could probably make a version that uses expressions like *aNext++ and eliminates the local variables that would be a lot less confusing. 926727 4 giles_joplin 2013-09-09 09:10:57 -0700 (In reply to comment #3) > (From update of attachment 210673 [details]) > The coding style here is awkward. The names “a/b increment” are not so good. I would call them shouldIncrementA/B instead. Also, the logic seems unnecessarily twisted. I have to read it over and over again to be sure it’s right. Might be worth another look to see if we can make the logic clearer. I think we could probably make a version that uses expressions like *aNext++ and eliminates the local variables that would be a lot less confusing. I used aIncrement and bIncrement instead of names that make more sense as English, to emphasize the relationship between aValue, aNext, and aIncrement. I agree that the logic is difficult to follow (it's better than it once was, if that's any consolation). I will take another crack and making it clearer (and hopefully a little shorter) in another patch. Thanks for the review. 926740 5 210673 commit-queue 2013-09-09 09:34:41 -0700 Comment on attachment 210673 Patch Clearing flags on attachment: 210673 Committed r155354: <http://trac.webkit.org/changeset/155354> 926741 6 commit-queue 2013-09-09 09:34:43 -0700 All reviewed patches have been landed. Closing bug. 928897 7 giles_joplin 2013-09-13 10:26:17 -0700 (In reply to comment #3) > (From update of attachment 210673 [details]) > The coding style here is awkward. The names “a/b increment” are not so good. I would call them shouldIncrementA/B instead. Also, the logic seems unnecessarily twisted. I have to read it over and over again to be sure it’s right. Might be worth another look to see if we can make the logic clearer. I think we could probably make a version that uses expressions like *aNext++ and eliminates the local variables that would be a lot less confusing. I took a run at this. The loop isn't simple enough to just use expressions like *aNext++ so I tried moving the fussy logic about safely managing a mutable interval value per iterator to a separate class: class SubtractShapeIntervalsIterator { public: SubtractShapeIntervalsIterator (const ShapeIntervals& intervals) : m_intervals(intervals) , m_nextInterval(intervals.begin()) { if (!m_intervals.isEmpty()) m_value = *m_nextInterval; } const_iterator shapeIntervalsIterator() const { return m_nextInterval; } bool isEnd() const { return m_intervals.isEmpty() || m_nextInterval == m_intervals.end(); } void next() { if (!m_intervals.isEmpty() && ++m_nextInterval != m_intervals.end()) m_value = *m_nextInterval; } ShapeInterval& value() { ASSERT(!m_intervals.empty() && m_nextInterval != m_intervals.end()); return m_value; } private: const ShapeIntervals& m_intervals; const_iterator m_nextInterval; ShapeInterval m_value; }; Rewriting the loop in terms of this private class makes it a little shorter, but sadly I don't think it's simpler or even more readable. while(!aIterator.isEnd() && !bIterator.isEnd()) { if (bIterator.value().contains(aIterator.value())) { aIterator.next(); } else if (aIterator.value().contains(bIterator.value())) { if (bIterator.value().x1() > aIterator.value().x1()) result.push_back(ShapeInterval<T>(aIterator.value().x1(), bIterator.value().x1())); if (aIterator.value().x2() > bIterator.value().x2()) aIterator.value().setX1(bIterator.value().x2()); else aIterator.next(); bIterator.next(); } else if (aIterator.value().overlaps(bIterator.value())) { if (aIterator.value() < bIterator.value()) { result.push_back(ShapeInterval<T>(aIterator.value().x1(), bIterator.value().x1())); aIterator.next(); } else { aIterator.value().setX1(bIterator.value().x2()); bIterator.next(); } } else { if (aIterator.value() < bIterator.value()) { result.push_back(aIterator.value()); aIterator.next(); } else bIterator.next(); } } if (!aIterator.isEnd()) { result.push_back(aIterator.value()); result.insert(result.end(), aIterator.shapeIntervalsIterator() + 1, a.end()); } 210670 2013-09-05 15:22:50 -0700 2013-09-05 15:22:50 -0700 Test case. fuzzer-test-case.html text/html 153 giles_joplin PHN0eWxlPgogICAgI3NoYXBlLWluc2lkZSB7CgogICAgICAgIC13ZWJraXQtc2hhcGUtaW5zaWRl OiBwb2x5Z29uKDBweCAwcHgsIDQwMHB4IDBweCwgMjAwcHggMTc1cHgpOwo8L3N0eWxlPgo8ZGl2 IGlkPSJzaGFwZS1pbnNpZGUiPgogICAgICAgIDxwb2x5Z29uPgoK 210673 2013-09-05 15:43:23 -0700 2013-09-09 09:34:41 -0700 Patch tmp.patch text/plain 5319 giles_joplin ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCBiMDhiNmQ4Li44NWI2NGM0IDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9DaGFuZ2VM b2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTMtMDkt MDUgIEhhbnMgTXVsbGVyICA8aG11bGxlckBhZG9iZS5jb20+CisKKyAgICAgICAgW0NTUyBTaGFw ZXNdIEhlYXAtYnVmZmVyLW92ZXJmbG93IGluIFdlYkNvcmU6OlNoYXBlSW50ZXJ2YWw8ZmxvYXQ+ OjpzdWJ0cmFjdFNoYXBlSW50ZXJ2YWxzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xMjA4MDIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBBZGRlZCBhIHRlc3QgY2FzZSB0aGF0IGNyYXNoZWQgYSBib3VuZHMt Y2hlY2tpbmcgcnVudGltZSBwcmlvciB0byB0aGlzIGZpeC4KKworICAgICAgICAqIGZhc3Qvc2hh cGVzL3NoYXBlLWluc2lkZS9zaGFwZS1pbnNpZGUtc3VidHJhY3QtaW50ZXJ2YWxzLWNyYXNoLWV4 cGVjdGVkLmh0bWw6IEFkZGVkLgorICAgICAgICAqIGZhc3Qvc2hhcGVzL3NoYXBlLWluc2lkZS9z aGFwZS1pbnNpZGUtc3VidHJhY3QtaW50ZXJ2YWxzLWNyYXNoLmh0bWw6IEFkZGVkLgorCiAyMDEz LTA5LTA0ICBEZWFuIEphY2tzb24gIDxkaW5vQGFwcGxlLmNvbT4KIAogICAgICAgICBSZW5hbWUg c3VwcG9ydHNDb250ZXh0IHRvIHByb2JhYmx5U3VwcG9ydHNDb250ZXh0CmRpZmYgLS1naXQgYS9M YXlvdXRUZXN0cy9mYXN0L3NoYXBlcy9zaGFwZS1pbnNpZGUvc2hhcGUtaW5zaWRlLXN1YnRyYWN0 LWludGVydmFscy1jcmFzaC1leHBlY3RlZC5odG1sIGIvTGF5b3V0VGVzdHMvZmFzdC9zaGFwZXMv c2hhcGUtaW5zaWRlL3NoYXBlLWluc2lkZS1zdWJ0cmFjdC1pbnRlcnZhbHMtY3Jhc2gtZXhwZWN0 ZWQuaHRtbApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAwMDAwLi4xZDJmMjViCi0tLSAv ZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMvZmFzdC9zaGFwZXMvc2hhcGUtaW5zaWRlL3NoYXBl LWluc2lkZS1zdWJ0cmFjdC1pbnRlcnZhbHMtY3Jhc2gtZXhwZWN0ZWQuaHRtbApAQCAtMCwwICsx LDE0IEBACis8IURPQ1RZUEUgaHRtbD4KKzxodG1sPgorPGJvZHk+Cis8cD4KK1RoaXMgdGVzdCBj YXVzZWQgYSBjcmFzaCB3aGVuIHJ1biB3aXRoIGEgYm91bmRzIGNoZWNrZXIsIHNlZSBodHRwczov L2NvZGUuZ29vZ2xlLmNvbS9wL2Nocm9taXVtL2lzc3Vlcy9kZXRhaWw/aWQ9Mjg1Nzg4LgorPHBy ZT4KK0Z1enplcjogSW5mZXJub19sYXlvdXRfdGVzdF9mdXp6ZXIKK0pvYiBUeXBlOiBMaW51eF9h c2FuX2NvbnRlbnRfc2hlbGxfZHJ0CitDcmFzaCBUeXBlOiBIZWFwLWJ1ZmZlci1vdmVyZmxvdyBS RUFEIDgKKzwvcHJlPgorPC9wPgorPGRpdj48L2Rpdj4KKzwvYm9keT4KKzwvaHRtbD4KZGlmZiAt LWdpdCBhL0xheW91dFRlc3RzL2Zhc3Qvc2hhcGVzL3NoYXBlLWluc2lkZS9zaGFwZS1pbnNpZGUt c3VidHJhY3QtaW50ZXJ2YWxzLWNyYXNoLmh0bWwgYi9MYXlvdXRUZXN0cy9mYXN0L3NoYXBlcy9z aGFwZS1pbnNpZGUvc2hhcGUtaW5zaWRlLXN1YnRyYWN0LWludGVydmFscy1jcmFzaC5odG1sCm5l dyBmaWxlIG1vZGUgMTAwNjQ0CmluZGV4IDAwMDAwMDAuLjgyYzAyYzgKLS0tIC9kZXYvbnVsbAor KysgYi9MYXlvdXRUZXN0cy9mYXN0L3NoYXBlcy9zaGFwZS1pbnNpZGUvc2hhcGUtaW5zaWRlLXN1 YnRyYWN0LWludGVydmFscy1jcmFzaC5odG1sCkBAIC0wLDAgKzEsMjEgQEAKKzwhRE9DVFlQRSBo dG1sPgorPGh0bWw+Cis8aGVhZD4KKzxzdHlsZT4KKyAgICAjc2hhcGUtaW5zaWRlIHsKKyAgICAg ICAgLXdlYmtpdC1zaGFwZS1pbnNpZGU6IHBvbHlnb24oMHB4IDBweCwgNDAwcHggMHB4LCAyMDBw eCAxNzVweCk7CisgICAgfQorPC9zdHlsZT4KKzwvaGVhZD4KKzxib2R5PgorPHA+CitUaGlzIHRl c3QgY2F1c2VkIGEgY3Jhc2ggd2hlbiBydW4gd2l0aCBhIGJvdW5kcyBjaGVja2VyLCBzZWUgaHR0 cHM6Ly9jb2RlLmdvb2dsZS5jb20vcC9jaHJvbWl1bS9pc3N1ZXMvZGV0YWlsP2lkPTI4NTc4OC4K KzxwcmU+CitGdXp6ZXI6IEluZmVybm9fbGF5b3V0X3Rlc3RfZnV6emVyCitKb2IgVHlwZTogTGlu dXhfYXNhbl9jb250ZW50X3NoZWxsX2RydAorQ3Jhc2ggVHlwZTogSGVhcC1idWZmZXItb3ZlcmZs b3cgUkVBRCA4Cis8L3ByZT4KKzwvcD4KKzxkaXYgaWQ9InNoYXBlLWluc2lkZSI+PC9kaXY+Cis8 L2JvZHk+Cis8L2h0bWw+CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cgYi9T b3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDE5YmI2Yy4uZDIyMDUyNSAxMDA2NDQKLS0t IGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxv ZwpAQCAtMSwzICsxLDE4IEBACisyMDEzLTA5LTA1ICBIYW5zIE11bGxlciAgPGhtdWxsZXJAYWRv YmUuY29tPgorCisgICAgICAgIFtDU1MgU2hhcGVzXSBIZWFwLWJ1ZmZlci1vdmVyZmxvdyBpbiBX ZWJDb3JlOjpTaGFwZUludGVydmFsPGZsb2F0Pjo6c3VidHJhY3RTaGFwZUludGVydmFscworICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTIwODAyCisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgUmV2aXNlZCB0aGUg aW1wbGVtZW50YXRpb24gb2Ygc3VidHJhY3RTaGFwZUludGVydmFscygpIHRvIGlzbG9hdGUgYW5k IGNoZWNrIHRoZQorICAgICAgICBwbGFjZXMgd2hlcmUgaXQgZGVyZWZlcmVuY2VzIFNoYXBlSW50 ZXJ2YWwgdmVjdG9yIGl0ZXJhdG9ycy4KKworICAgICAgICBUZXN0OiBmYXN0L3NoYXBlcy9zaGFw ZS1pbnNpZGUvc2hhcGUtaW5zaWRlLXN1YnRyYWN0LWludGVydmFscy1jcmFzaC5odG1sCisKKyAg ICAgICAgKiByZW5kZXJpbmcvc2hhcGVzL1NoYXBlSW50ZXJ2YWwuaDoKKyAgICAgICAgKFdlYkNv cmU6OlNoYXBlSW50ZXJ2YWw6OnN1YnRyYWN0U2hhcGVJbnRlcnZhbHMpOgorCiAyMDEzLTA5LTA1 ICBBbmRyZWFzIEtsaW5nICA8YWtsaW5nQGFwcGxlLmNvbT4KIAogICAgICAgICBDYWNoZWQgUGFn ZSBhbmQgRnJhbWUgZG9uJ3QgbmVlZCB0byBiZSByZWYtY291bnRlZC4KZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL3JlbmRlcmluZy9zaGFwZXMvU2hhcGVJbnRlcnZhbC5oIGIvU291cmNlL1dl YkNvcmUvcmVuZGVyaW5nL3NoYXBlcy9TaGFwZUludGVydmFsLmgKaW5kZXggZDFiNmRkMi4uYmYw YmMwOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL3NoYXBlcy9TaGFwZUlu dGVydmFsLmgKKysrIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL3NoYXBlcy9TaGFwZUludGVy dmFsLmgKQEAgLTE1NywzMyArMTU3LDQyIEBAIHB1YmxpYzoKICAgICAgICAgU2hhcGVJbnRlcnZh bDxUPiBhVmFsdWUgPSAqYU5leHQ7CiAgICAgICAgIFNoYXBlSW50ZXJ2YWw8VD4gYlZhbHVlID0g KmJOZXh0OwogCi0gICAgICAgIHdoaWxlIChhTmV4dCAhPSBhLmVuZCgpICYmIGJOZXh0ICE9IGIu ZW5kKCkpIHsKKyAgICAgICAgZG8geworICAgICAgICAgICAgYm9vbCBhSW5jcmVtZW50ID0gZmFs c2U7CisgICAgICAgICAgICBib29sIGJJbmNyZW1lbnQgPSBmYWxzZTsKKwogICAgICAgICAgICAg aWYgKGJWYWx1ZS5jb250YWlucyhhVmFsdWUpKSB7Ci0gICAgICAgICAgICAgICAgYVZhbHVlID0g KisrYU5leHQ7CisgICAgICAgICAgICAgICAgYUluY3JlbWVudCA9IHRydWU7CiAgICAgICAgICAg ICB9IGVsc2UgaWYgKGFWYWx1ZS5jb250YWlucyhiVmFsdWUpKSB7CiAgICAgICAgICAgICAgICAg aWYgKGJWYWx1ZS54MSgpID4gYVZhbHVlLngxKCkpCiAgICAgICAgICAgICAgICAgICAgIHJlc3Vs dC5hcHBlbmQoU2hhcGVJbnRlcnZhbDxUPihhVmFsdWUueDEoKSwgYlZhbHVlLngxKCkpKTsKICAg ICAgICAgICAgICAgICBpZiAoYVZhbHVlLngyKCkgPiBiVmFsdWUueDIoKSkKICAgICAgICAgICAg ICAgICAgICAgYVZhbHVlLnNldFgxKGJWYWx1ZS54MigpKTsKICAgICAgICAgICAgICAgICBlbHNl Ci0gICAgICAgICAgICAgICAgICAgIGFWYWx1ZSA9ICorK2FOZXh0OwotICAgICAgICAgICAgICAg IGJWYWx1ZSA9ICorK2JOZXh0OworICAgICAgICAgICAgICAgICAgICBhSW5jcmVtZW50ID0gdHJ1 ZTsKKyAgICAgICAgICAgICAgICBiSW5jcmVtZW50ID0gdHJ1ZTsKICAgICAgICAgICAgIH0gZWxz ZSBpZiAoYVZhbHVlLm92ZXJsYXBzKGJWYWx1ZSkpIHsKICAgICAgICAgICAgICAgICBpZiAoYVZh bHVlLngxKCkgPCBiVmFsdWUueDEoKSkgewogICAgICAgICAgICAgICAgICAgICByZXN1bHQuYXBw ZW5kKFNoYXBlSW50ZXJ2YWw8VD4oYVZhbHVlLngxKCksIGJWYWx1ZS54MSgpKSk7Ci0gICAgICAg ICAgICAgICAgICAgIGFWYWx1ZSA9ICorK2FOZXh0OworICAgICAgICAgICAgICAgICAgICBhSW5j cmVtZW50ID0gdHJ1ZTsKICAgICAgICAgICAgICAgICB9IGVsc2UgewogICAgICAgICAgICAgICAg ICAgICBhVmFsdWUuc2V0WDEoYlZhbHVlLngyKCkpOwotICAgICAgICAgICAgICAgICAgICBiVmFs dWUgPSAqKytiTmV4dDsKKyAgICAgICAgICAgICAgICAgICAgYkluY3JlbWVudCA9IHRydWU7CiAg ICAgICAgICAgICAgICAgfQogICAgICAgICAgICAgfSBlbHNlIHsKICAgICAgICAgICAgICAgICBp ZiAoYVZhbHVlLngxKCkgPCBiVmFsdWUueDEoKSkgewogICAgICAgICAgICAgICAgICAgICByZXN1 bHQuYXBwZW5kKGFWYWx1ZSk7Ci0gICAgICAgICAgICAgICAgICAgIGFWYWx1ZSA9ICorK2FOZXh0 OworICAgICAgICAgICAgICAgICAgICBhSW5jcmVtZW50ID0gdHJ1ZTsKICAgICAgICAgICAgICAg ICB9IGVsc2UKLSAgICAgICAgICAgICAgICAgICAgYlZhbHVlID0gKisrYk5leHQ7CisgICAgICAg ICAgICAgICAgICAgIGJJbmNyZW1lbnQgPSB0cnVlOwogICAgICAgICAgICAgfQotICAgICAgICB9 CisKKyAgICAgICAgICAgIGlmIChhSW5jcmVtZW50ICYmICsrYU5leHQgIT0gYS5lbmQoKSkKKyAg ICAgICAgICAgICAgICBhVmFsdWUgPSAqYU5leHQ7CisgICAgICAgICAgICBpZiAoYkluY3JlbWVu dCAmJiArK2JOZXh0ICE9IGIuZW5kKCkpCisgICAgICAgICAgICAgICAgYlZhbHVlID0gKmJOZXh0 OworCisgICAgICAgIH0gd2hpbGUgKGFOZXh0ICE9IGEuZW5kKCkgJiYgYk5leHQgIT0gYi5lbmQo KSk7CiAKICAgICAgICAgaWYgKGFOZXh0ICE9IGEuZW5kKCkpIHsKICAgICAgICAgICAgIHJlc3Vs dC5hcHBlbmQoYVZhbHVlKTsK