Last Comment Bug 735647 - ReflowCountMgr risks overrunning stack and heap buffers on 64-bit systems
: ReflowCountMgr risks overrunning stack and heap buffers on 64-bit systems
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Misc Code (show other bugs)
: Trunk
: x86_64 All
: -- normal (vote)
: mozilla14
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-14 06:16 PDT by Jonathan Kew (:jfkthame)
Modified: 2012-03-16 05:54 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, use larger buffers for key strings in ReflowCountMgr (2.48 KB, patch)
2012-03-14 06:16 PDT, Jonathan Kew (:jfkthame)
mats: review+
Details | Diff | Review
patch, use larger buffers for key strings in ReflowCountMgr, and fix leakage & allocator mismatch (3.53 KB, patch)
2012-03-14 09:15 PDT, Jonathan Kew (:jfkthame)
mats: review+
Details | Diff | Review

Description Jonathan Kew (:jfkthame) 2012-03-14 06:16:31 PDT
Created attachment 605721 [details] [diff] [review]
patch, use larger buffers for key strings in ReflowCountMgr

Several times, the ReflowCountMgr code uses sprintf(key, "%p", ...) to turn a pointer into a "key" string. But it uses 16-byte buffers for this, which could be overrun on a 64-bit system where a pointer may need up to 19 chars to sprintf successfully.

Good thing this isn't enabled in shipping builds, AFAIK. :)

The patch enlarges these buffers from 16 to 24 chars, which should be enough for now.
Comment 1 Mats Palmgren (:mats) 2012-03-14 08:12:46 PDT
Comment on attachment 605721 [details] [diff] [review]
patch, use larger buffers for key strings in ReflowCountMgr

Hmm, does the two places that use "new char[16]" leak that string?
ReflowCountMgr::PaintCount always leaks AFAICT.
ReflowCountMgr::Add leaks in case PL_HashTableAdd isn't executed.
Comment 2 Jonathan Kew (:jfkthame) 2012-03-14 09:02:43 PDT
(In reply to Mats Palmgren [:mats] from comment #1)
> Comment on attachment 605721 [details] [diff] [review]
> patch, use larger buffers for key strings in ReflowCountMgr
> 
> Hmm, does the two places that use "new char[16]" leak that string?
> ReflowCountMgr::PaintCount always leaks AFAICT.
> ReflowCountMgr::Add leaks in case PL_HashTableAdd isn't executed.

Yeah, I think you're right.

Moreover, it also looks like there's an allocator mismatch in the hashtable: the "key" string is allocated using new[], but then in ReflowCountMgr::RemoveIndiItems, it's deleted using NS_Free.

PaintCount could just use a stack buffer, if it doesn't need to hand it over to the hashtable.

And in Add, we could use a stack buffer initially, and then NS_strdup it in the case where it needs to be added to the hash.

Guess I'll post a followup...
Comment 3 Jonathan Kew (:jfkthame) 2012-03-14 09:15:23 PDT
Created attachment 605789 [details] [diff] [review]
patch, use larger buffers for key strings in ReflowCountMgr, and fix leakage & allocator mismatch

Updated patch to include fixes for the issues we just mentioned.
Comment 4 Mats Palmgren (:mats) 2012-03-14 10:37:54 PDT
Comment on attachment 605789 [details] [diff] [review]
patch, use larger buffers for key strings in ReflowCountMgr, and fix leakage & allocator mismatch

>      sprintf(key, "%p", (void*)aFrame);

I really don't understand why we use a string as hash key for a frame
pointer in the first place.  I think we could just use NS_PTR_TO_INT32.

It's just debug code, so it's not important, but please file a new bug
unless you want to fix it here.
Comment 5 Jonathan Kew (:jfkthame) 2012-03-15 02:23:46 PDT
I don't really want to make further changes here and now - I just happened to notice some dangerous-looking code in passing and didn't want to leave it there as a potential landmine.

So pushed this to inbound to fix the immediate errors; filed bug 736015 as a followup about eliminating the stringification.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b34a33b13110
Comment 6 Marco Bonardo [::mak] 2012-03-16 05:54:51 PDT
https://hg.mozilla.org/mozilla-central/rev/b34a33b13110

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