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

RESOLVED FIXED in mozilla14

Status

()

Core
Layout: Misc Code
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

Trunk
mozilla14
x86_64
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
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[16]" leak that string?
ReflowCountMgr::PaintCount always leaks AFAICT.
ReflowCountMgr::Add leaks in case PL_HashTableAdd isn't executed.
Attachment #605721 - Flags: review?(matspal) → review+
(Assignee)

Comment 2

5 years ago
(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...
(Assignee)

Comment 3

5 years ago
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.
Assignee: nobody → jfkthame
Attachment #605721 - Attachment is obsolete: true
Attachment #605789 - Flags: review?(matspal)
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+
(Assignee)

Comment 5

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/b34a33b13110
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.