735647 - ReflowCountMgr risks overrunning stack and heap buffers on 64-bit systems

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: 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 (inactive)]

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]]

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

Attachment: 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 (inactive)]

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]]

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]]

https://hg.mozilla.org/mozilla-central/rev/b34a33b13110