Bug 26449 - UMR in WebCore::BitStack
Summary: UMR in WebCore::BitStack
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-16 10:53 PDT by Tony Chang
Modified: 2009-06-16 14:53 PDT (History)
1 user (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2009-06-16 10:53:08 PDT
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 Tony Chang 2009-06-16 10:58:32 PDT
Created attachment 31354 [details]
v1
Comment 2 Darin Fisher (:fishd, Google) 2009-06-16 11:08:51 PDT
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
Comment 3 Tony Chang 2009-06-16 11:15:32 PDT
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 4 Darin Fisher (:fishd, Google) 2009-06-16 12:50:11 PDT
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
Comment 5 Dimitri Glazkov (Google) 2009-06-16 13:32:13 PDT
Landed as http://trac.webkit.org/changeset/44735.
Comment 6 Darin Adler 2009-06-16 14:40:32 PDT
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 Tony Chang 2009-06-16 14:46:06 PDT
(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 Darin Adler 2009-06-16 14:48:07 PDT
(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 Tony Chang 2009-06-16 14:53:26 PDT
(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.