34156 2010-01-26 00:56:28 -0800 Incorrect boolean expression in isMailBlockquote() (WebCore/htmlediting.cpp) 2010-01-29 13:42:39 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 rolandsteiner webkit-unassigned darin oldest_to_newest 184518 0 rolandsteiner 2010-01-26 00:56:28 -0800 Incorrect boolean expression in isMailBlockquote() (WebCore/htmlediting.cpp) 184519 1 47391 rolandsteiner 2010-01-26 00:59:28 -0800 Created attachment 47391 Patch 184520 2 rolandsteiner 2010-01-26 01:01:03 -0800 Could potentially lead to a crash if applied to an attribute named "blockquote", or an incorrect result if applied to a different element that has an attribute "cite". 184596 3 47391 darin 2010-01-26 08:18:36 -0800 Comment on attachment 47391 Patch > + Could potentially lead to a crash if applied to an attribute named "blockquote", That's incorrect. The Node::hasTagName will only return true for an Element. You can look at the implementation to double-check this. > + or an incorrect result if applied to a different element that has an attribute "cite". I don't understand this claim. > + No new tests. (minor code change) If the "cite" case existed, you could probably construct a test case for it. > - if (!node || (!node->isElementNode() && !node->hasTagName(blockquoteTag))) > + if (!node || !node->isElementNode() || !node->hasTagName(blockquoteTag)) > return false; The isElementNode check is entirely unnecessary and can be removed. That's probably the best fix. 184878 4 47478 rolandsteiner 2010-01-26 18:01:52 -0800 Created attachment 47478 patch - simplified expression 185184 5 darin 2010-01-27 12:21:29 -0800 It’s too late for this, I guess, but I realized that there must be a real bug here. There *must* have been some way to get this code to run for non-blockquote elements just by adding a cite attribute to them, and this in turn must have done the wrong thing in the editing operation. If we dig into that a bit I think we could reproduce incorrect behavior, although probably not a crash. So a test case would have been possible and should have been developed. 185333 6 rolandsteiner 2010-01-27 21:09:24 -0800 Yes (btw, that was what I was referring to in my remark, was just poorly worded). I'll probably continue to be working on editing anyway, so I'll keep it in the back of my head and try to come up with (or include it in) a good test case. 185334 7 rolandsteiner 2010-01-27 21:11:09 -0800 (forgot: patch was committed in rev. 53885) 185949 8 darin 2010-01-29 13:42:39 -0800 (In reply to comment #6) > Yes (btw, that was what I was referring to in my remark, was just poorly > worded). Makes sense. I eventually figured that out. 47391 2010-01-26 00:59:28 -0800 2010-01-26 18:01:47 -0800 Patch bug-34156-20100126175926.patch text/plain 1411 rolandsteiner SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1Mzg0MikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTggQEAKKzIwMTAtMDEtMjYgIFJvbGFuZCBTdGVpbmVyICA8cm9sYW5kc3RlaW5l ckBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgSW5jb3JyZWN0IGJvb2xlYW4gZXhwcmVzc2lvbiBpbiBpc01haWxCbG9ja3F1b3Rl KCkgKFdlYkNvcmUvaHRtbGVkaXRpbmcuY3BwKQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzQxNTYKKworICAgICAgICBDb3VsZCBwb3RlbnRpYWxseSBs ZWFkIHRvIGEgY3Jhc2ggaWYgYXBwbGllZCB0byBhbiBhdHRyaWJ1dGUgbmFtZWQgImJsb2NrcXVv dGUiLAorICAgICAgICBvciBhbiBpbmNvcnJlY3QgcmVzdWx0IGlmIGFwcGxpZWQgdG8gYSBkaWZm ZXJlbnQgZWxlbWVudCB0aGF0IGhhcyBhbiBhdHRyaWJ1dGUgImNpdGUiLgorCisgICAgICAgIE5v IG5ldyB0ZXN0cy4gKG1pbm9yIGNvZGUgY2hhbmdlKQorCisgICAgICAgICogZWRpdGluZy9odG1s ZWRpdGluZy5jcHA6CisgICAgICAgIChXZWJDb3JlOjppc01haWxCbG9ja3F1b3RlKToKKwogMjAx MC0wMS0yNiAgQW5kcmFzIEJlY3NpICA8YWJlY3NpQGluZi51LXN6ZWdlZC5odT4KIAogICAgICAg ICBVbnJldmlld2VkIGJ1aWxkIGZpeC4KSW5kZXg6IFdlYkNvcmUvZWRpdGluZy9odG1sZWRpdGlu Zy5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9lZGl0aW5nL2h0bWxlZGl0aW5nLmNwcAkocmV2 aXNpb24gNTM4MzUpCisrKyBXZWJDb3JlL2VkaXRpbmcvaHRtbGVkaXRpbmcuY3BwCSh3b3JraW5n IGNvcHkpCkBAIC05ODMsNyArOTgzLDcgQEAgdW5zaWduZWQgbnVtRW5jbG9zaW5nTWFpbEJsb2Nr cXVvdGVzKGNvbgogCiBib29sIGlzTWFpbEJsb2NrcXVvdGUoY29uc3QgTm9kZSAqbm9kZSkKIHsK LSAgICBpZiAoIW5vZGUgfHwgKCFub2RlLT5pc0VsZW1lbnROb2RlKCkgJiYgIW5vZGUtPmhhc1Rh Z05hbWUoYmxvY2txdW90ZVRhZykpKQorICAgIGlmICghbm9kZSB8fCAhbm9kZS0+aXNFbGVtZW50 Tm9kZSgpIHx8ICFub2RlLT5oYXNUYWdOYW1lKGJsb2NrcXVvdGVUYWcpKQogICAgICAgICByZXR1 cm4gZmFsc2U7CiAgICAgICAgIAogICAgIHJldHVybiBzdGF0aWNfY2FzdDxjb25zdCBFbGVtZW50 ICo+KG5vZGUpLT5nZXRBdHRyaWJ1dGUoInR5cGUiKSA9PSAiY2l0ZSI7Cg== 47478 2010-01-26 18:01:52 -0800 2010-01-26 18:08:24 -0800 patch - simplified expression bug-34156-20100127110151.patch text/plain 1202 rolandsteiner SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1Mzg3NikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMTAtMDEtMjYgIFJvbGFuZCBTdGVpbmVyICA8cm9sYW5kc3RlaW5l ckBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgSW5jb3JyZWN0IGJvb2xlYW4gZXhwcmVzc2lvbiBpbiBpc01haWxCbG9ja3F1b3Rl KCkgKFdlYkNvcmUvaHRtbGVkaXRpbmcuY3BwKQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzQxNTYKKworICAgICAgICBObyBuZXcgdGVzdHMgKG1pbm9y IGNvZGUgY2hhbmdlKS4KKworICAgICAgICAqIGVkaXRpbmcvaHRtbGVkaXRpbmcuY3BwOgorICAg ICAgICAoV2ViQ29yZTo6aXNNYWlsQmxvY2txdW90ZSk6CisKIDIwMTAtMDEtMjYgIERtaXRyeSBU aXRvdiAgPGRpbWljaEBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgU3RldmUg RmFsa2VuYnVyZy4KSW5kZXg6IFdlYkNvcmUvZWRpdGluZy9odG1sZWRpdGluZy5jcHAKPT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PQotLS0gV2ViQ29yZS9lZGl0aW5nL2h0bWxlZGl0aW5nLmNwcAkocmV2aXNpb24gNTM4NzYp CisrKyBXZWJDb3JlL2VkaXRpbmcvaHRtbGVkaXRpbmcuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC05 ODMsNyArOTgzLDcgQEAgdW5zaWduZWQgbnVtRW5jbG9zaW5nTWFpbEJsb2NrcXVvdGVzKGNvbgog CiBib29sIGlzTWFpbEJsb2NrcXVvdGUoY29uc3QgTm9kZSAqbm9kZSkKIHsKLSAgICBpZiAoIW5v ZGUgfHwgKCFub2RlLT5pc0VsZW1lbnROb2RlKCkgJiYgIW5vZGUtPmhhc1RhZ05hbWUoYmxvY2tx dW90ZVRhZykpKQorICAgIGlmICghbm9kZSB8fCAhbm9kZS0+aGFzVGFnTmFtZShibG9ja3F1b3Rl VGFnKSkKICAgICAgICAgcmV0dXJuIGZhbHNlOwogICAgICAgICAKICAgICByZXR1cm4gc3RhdGlj X2Nhc3Q8Y29uc3QgRWxlbWVudCAqPihub2RlKS0+Z2V0QXR0cmlidXRlKCJ0eXBlIikgPT0gImNp dGUiOwo=