553586 - Heap corruption in windows ShowNativePrintDialog

Status: RESOLVED FIXED

(Core :: Printing: Setup, defect)

Description

[Jacek Caban]

Attachment: fix

While filling DEVNAMES structure, we overwrite allocated buffer. An offset is set to sizeof(DEVNAMES):

pDevNames->wDriverOffset = sizeof(DEVNAMES);

The offset is supposed to be set in chars (wchar_t in this case). We tread it as offset later:

wchar_t* device = &(((wchar_t*)pDevNames)[pDevNames->wDeviceOffset]);

but the offset doesn't point at the end of an DEVNAMES structure, so there is an overwrite. The DEVNAMES structure contains four WORDs, so it's safe to set an offset to sizeof(DEVNAMES)/sizeof(wchar_t).

There is another bug in wOutputOffset:
pDevNames->wOutputOffset = sizeof(DEVNAMES)+len+1;
It points to the place *after* filled buffer, but it was probably meant to point to the null byte, so the +1 is wrong.

The attached patch fixes the problem and cleans up a little (a simple memcpy is way cleaner than current addres-of-array-element magic IMO).

While I was at this I've also fixed a few compiler warnings about unused variables and const char* -> char* casts.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 433554 [details] [diff] [review]
fix

could've sworn I r+'d this already, sorry!  Really wish this wasn't as magic as it looks, but the DEVNAMES API looks awful to begin with.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/8c573f37efba