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.
Created attachment 31354 [details] v1
Comment on attachment 31354 [details] 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
Created attachment 31355 [details] 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?
Comment on attachment 31355 [details] 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
Landed as http://trac.webkit.org/changeset/44735.
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.
(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.
(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.
(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.