26449 2009-06-16 10:53:08 -0700 UMR in WebCore::BitStack 2009-06-16 14:53:26 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 tony tony darin oldest_to_newest 126227 0 tony 2009-06-16 10:53:08 -0700 WebCore::BitStack was added to TextIterator in r44674. Purify is reporting a UMR: Uninitialized memory read in WebCore::BitStack::push(bool) Error Location third_party/webkit/webcore/editing/textiterator.cpp WebCore::BitStack::push(bool) third_party/webkit/webcore/editing/textiterator.cpp WebCore::pushFullyClippedState third_party/webkit/webcore/editing/textiterator.cpp WebCore::setUpFullyClippedStack third_party/webkit/webcore/editing/textiterator.cpp WebCore::TextIterator::TextIterator(Range::WebCore const*,bool,bool) third_party/webkit/webcore/editing/textiterator.cpp WebCore::plainTextToMallocAllocatedBuffer(class WebCore::Range const *,unsigned int &,bool) third_party/webkit/webcore/editing/textiterator.cpp WebCore::plainText(Range::WebCore const*) third_party/webkit/webcore/dom/element.cpp WebCore::Element::innerText(void)const webkit/glue/webkit_glue.cc webkit_glue::DumpDocumentText(class WebFrame *) webkit/tools/test_shell/test_shell.cc TestShell::GetDocumentText(void) webkit/glue/bookmarklet_unittest.cc BookmarkletTest_DocumentWrite_Test::TestBody(void) testing/gtest/src/gtest.cc testing::Test::Run(void) ^^^ -- It looks like when we grow |m_words|, we don't initialize the new memory which we then use as |word| causing a UMR. I guess we just need to assign 0 to the new memory. Patch coming soon, but feel free to check in before me. 126231 1 31354 tony 2009-06-16 10:58:32 -0700 Created attachment 31354 v1 126233 2 31354 fishd 2009-06-16 11:08:51 -0700 Comment on attachment 31354 v1 > Index: WebCore/ChangeLog ... > +2009-06-16 tony chang <tony@chromium.org> nit: people normally use mixed case for their names. > + Reviewed by NOBODY (OOPS!). > + > + WARNING: NO TEST CASES ADDED OR CHANGED > + > + Fix a UMR in WebCore::BitStack by initializing new memory to 0. > + https://bugs.webkit.org/show_bug.cgi?id=26449 > + No new tests, covered by purify. > + nit: please be sure to add a bug link in the ChangeLog before committing. Otherwise, R=me 126237 3 31355 tony 2009-06-16 11:15:32 -0700 Created attachment 31355 v2 Updated to use capitals in my name. The changelog had the bug link already :). Also, I don't have the commit bit, can you land this for me? 126264 4 31355 fishd 2009-06-16 12:50:11 -0700 Comment on attachment 31355 v2 > Index: WebCore/ChangeLog ... > +2009-06-16 Tony Chang <tony@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + WARNING: NO TEST CASES ADDED OR CHANGED > + > + Fix a UMR in WebCore::BitStack by initializing new memory to 0. > + https://bugs.webkit.org/show_bug.cgi?id=26449 > + No new tests, covered by purify. > + Oh, sorry... I'm used to see the bug reference adjacent to the text describing the change. You'll want to kill the WARNING line too before committing. Still R=me 126275 5 dglazkov 2009-06-16 13:32:13 -0700 Landed as http://trac.webkit.org/changeset/44735. 126298 6 darin 2009-06-16 14:40:32 -0700 This is a false positive in Purify. The new words don't need to be initialized! I set each bit as I use it and don't rely on the old state. But I have no objection to making the code change to make Purify happy. 126305 7 tony 2009-06-16 14:46:06 -0700 (In reply to comment #6) > This is a false positive in Purify. The new words don't need to be initialized! > I set each bit as I use it and don't rely on the old state. But I have no > objection to making the code change to make Purify happy. Are you sure about that? 01 void BitStack::push(bool bit) 02 { 03 unsigned index = m_size / bitsInWord; 04 unsigned shift = m_size & bitInWordMask; 05 if (!shift && index == m_words.size()) 06 m_words.grow(index + 1); 07 unsigned& word = m_words[index]; 08 unsigned mask = 1U << shift; 09 if (bit) 10 word |= mask; 11 else 12 word &= ~mask; 13 ++m_size; 14 } In line 6 we grow the vector and don't give it a value. In line 7, we store the value in |word|. In line 10 or 12, we |= or &= which uses the uninitialized value. 126308 8 darin 2009-06-16 14:48:07 -0700 (In reply to comment #7) > Are you sure about that? I am. > 01 void BitStack::push(bool bit) > 02 { > 03 unsigned index = m_size / bitsInWord; > 04 unsigned shift = m_size & bitInWordMask; > 05 if (!shift && index == m_words.size()) > 06 m_words.grow(index + 1); > 07 unsigned& word = m_words[index]; > 08 unsigned mask = 1U << shift; > 09 if (bit) > 10 word |= mask; > 11 else > 12 word &= ~mask; > 13 ++m_size; > 14 } > > In line 6 we grow the vector and don't give it a value. In line 7, we store > the value in |word|. In line 10 or 12, we |= or &= which uses the > uninitialized value. I think it really depends on what you mean by "use" the value. The |= and &= set or clear the low bit, and that's the only bit that's used until we push more bits on the stack. The other bits can have any value. But there's really no point debating this. I'm fine with the code change. 126312 9 tony 2009-06-16 14:53:26 -0700 (In reply to comment #8) > I think it really depends on what you mean by "use" the value. The |= and &= > set or clear the low bit, and that's the only bit that's used until we push > more bits on the stack. The other bits can have any value. Ah, now I understand. Yes, you are correct. 31354 2009-06-16 10:58:32 -0700 2009-06-16 11:15:32 -0700 v1 umr-26449.diff text/plain 1266 tony SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NDcyNCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMDktMDYtMTYgIHRvbnkgY2hhbmcgIDx0b255QGNocm9taXVtLm9y Zz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXQVJO SU5HOiBOTyBURVNUIENBU0VTIEFEREVEIE9SIENIQU5HRUQKKyAgICAgICAgCisgICAgICAgIEZp eCBhIFVNUiBpbiBXZWJDb3JlOjpCaXRTdGFjayBieSBpbml0aWFsaXppbmcgbmV3IG1lbW9yeSB0 byAwLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjY0 NDkKKyAgICAgICAgTm8gbmV3IHRlc3RzLCBjb3ZlcmVkIGJ5IHB1cmlmeS4KKworICAgICAgICAq IGVkaXRpbmcvVGV4dEl0ZXJhdG9yLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkJpdFN0YWNrOjpw dXNoKToKKwogMjAwOS0wNi0xNiAgQnJlbnQgRnVsZ2hhbSAgPGJmdWxnaGFtQGdtYWlsLmNvbT4K IAogICAgICAgICBSZXZpZXdlZCBieSBEYXJpbiBBZGxlci4KSW5kZXg6IFdlYkNvcmUvZWRpdGlu Zy9UZXh0SXRlcmF0b3IuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvZWRpdGluZy9UZXh0SXRl cmF0b3IuY3BwCShyZXZpc2lvbiA0NDcyMykKKysrIFdlYkNvcmUvZWRpdGluZy9UZXh0SXRlcmF0 b3IuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMDksOCArMTA5LDEwIEBAIHZvaWQgQml0U3RhY2s6 OnB1c2goYm9vbCBiaXQpCiB7CiAgICAgdW5zaWduZWQgaW5kZXggPSBtX3NpemUgLyBiaXRzSW5X b3JkOwogICAgIHVuc2lnbmVkIHNoaWZ0ID0gbV9zaXplICYgYml0SW5Xb3JkTWFzazsKLSAgICBp ZiAoIXNoaWZ0ICYmIGluZGV4ID09IG1fd29yZHMuc2l6ZSgpKQorICAgIGlmICghc2hpZnQgJiYg aW5kZXggPT0gbV93b3Jkcy5zaXplKCkpIHsKICAgICAgICAgbV93b3Jkcy5ncm93KGluZGV4ICsg MSk7CisgICAgICAgIG1fd29yZHNbaW5kZXhdID0gMDsKKyAgICB9CiAgICAgdW5zaWduZWQmIHdv cmQgPSBtX3dvcmRzW2luZGV4XTsKICAgICB1bnNpZ25lZCBtYXNrID0gMVUgPDwgc2hpZnQ7CiAg ICAgaWYgKGJpdCkK 31355 2009-06-16 11:15:32 -0700 2009-06-16 12:50:11 -0700 v2 umr-26449-2.diff text/plain 1266 tony SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NDcyNCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMDktMDYtMTYgIFRvbnkgQ2hhbmcgIDx0b255QGNocm9taXVtLm9y Zz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXQVJO SU5HOiBOTyBURVNUIENBU0VTIEFEREVEIE9SIENIQU5HRUQKKyAgICAgICAgCisgICAgICAgIEZp eCBhIFVNUiBpbiBXZWJDb3JlOjpCaXRTdGFjayBieSBpbml0aWFsaXppbmcgbmV3IG1lbW9yeSB0 byAwLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjY0 NDkKKyAgICAgICAgTm8gbmV3IHRlc3RzLCBjb3ZlcmVkIGJ5IHB1cmlmeS4KKworICAgICAgICAq IGVkaXRpbmcvVGV4dEl0ZXJhdG9yLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkJpdFN0YWNrOjpw dXNoKToKKwogMjAwOS0wNi0xNiAgQnJlbnQgRnVsZ2hhbSAgPGJmdWxnaGFtQGdtYWlsLmNvbT4K IAogICAgICAgICBSZXZpZXdlZCBieSBEYXJpbiBBZGxlci4KSW5kZXg6IFdlYkNvcmUvZWRpdGlu Zy9UZXh0SXRlcmF0b3IuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvZWRpdGluZy9UZXh0SXRl cmF0b3IuY3BwCShyZXZpc2lvbiA0NDcyMykKKysrIFdlYkNvcmUvZWRpdGluZy9UZXh0SXRlcmF0 b3IuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMDksOCArMTA5LDEwIEBAIHZvaWQgQml0U3RhY2s6 OnB1c2goYm9vbCBiaXQpCiB7CiAgICAgdW5zaWduZWQgaW5kZXggPSBtX3NpemUgLyBiaXRzSW5X b3JkOwogICAgIHVuc2lnbmVkIHNoaWZ0ID0gbV9zaXplICYgYml0SW5Xb3JkTWFzazsKLSAgICBp ZiAoIXNoaWZ0ICYmIGluZGV4ID09IG1fd29yZHMuc2l6ZSgpKQorICAgIGlmICghc2hpZnQgJiYg aW5kZXggPT0gbV93b3Jkcy5zaXplKCkpIHsKICAgICAgICAgbV93b3Jkcy5ncm93KGluZGV4ICsg MSk7CisgICAgICAgIG1fd29yZHNbaW5kZXhdID0gMDsKKyAgICB9CiAgICAgdW5zaWduZWQmIHdv cmQgPSBtX3dvcmRzW2luZGV4XTsKICAgICB1bnNpZ25lZCBtYXNrID0gMVUgPDwgc2hpZnQ7CiAg ICAgaWYgKGJpdCkK