Closed Bug 735647 Opened 9 years ago Closed 9 years ago
Count Mgr risks overrunning stack and heap buffers on 64-bit systems
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.
Attachment #605721 - Flags: review?(matspal)
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" leak that string? ReflowCountMgr::PaintCount always leaks AFAICT. ReflowCountMgr::Add leaks in case PL_HashTableAdd isn't executed.
Attachment #605721 - Flags: review?(matspal) → review+
(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" 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...
Updated patch to include fixes for the issues we just mentioned.
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.
Attachment #605789 - Flags: review?(matspal) → review+
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
Target Milestone: --- → mozilla14
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.