Bug 26449 – UMR in WebCore::BitStack

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tony Chang]

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.

[Tony Chang]

Created attachment 31354 [details]
v1

[Darin Fisher (:fishd, Google)]

Comment on attachment 31354 [details]
v1

> Index: WebCore/ChangeLog
...
> [phone]  tony chang  <[e-mail]>

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

[Tony Chang]

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?

[Darin Fisher (:fishd, Google)]

Comment on attachment 31355 [details]
v2

> Index: WebCore/ChangeLog
...
> [phone]  Tony Chang  <[e-mail]>
> +
> +        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

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/44735.

[Darin Adler]

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.

[Tony Chang]

(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.

[Darin Adler]

(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.

[Tony Chang]

(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.