Bug 26449 - UMR in WebCore::BitStack
: UMR in WebCore::BitStack
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-06-16 10:53 PST by
Modified: 2009-06-16 14:53 PST (History)


Attachments
v1 (1.24 KB, patch)
2009-06-16 10:58 PST, Tony Chang
fishd: review+
Review Patch | Details | Formatted Diff | Diff
v2 (1.24 KB, patch)
2009-06-16 11:15 PST, Tony Chang
fishd: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-06-16 10:53:08 PST
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.
------- Comment #1 From 2009-06-16 10:58:32 PST -------
Created an attachment (id=31354) [details]
v1
------- Comment #2 From 2009-06-16 11:08:51 PST -------
(From update of attachment 31354 [details])
> 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
------- Comment #3 From 2009-06-16 11:15:32 PST -------
Created an attachment (id=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 #4 From 2009-06-16 12:50:11 PST -------
(From update of attachment 31355 [details])
> 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
------- Comment #5 From 2009-06-16 13:32:13 PST -------
Landed as http://trac.webkit.org/changeset/44735.
------- Comment #6 From 2009-06-16 14:40:32 PST -------
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.
------- Comment #7 From 2009-06-16 14:46:06 PST -------
(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.
------- Comment #8 From 2009-06-16 14:48:07 PST -------
(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.
------- Comment #9 From 2009-06-16 14:53:26 PST -------
(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.