Closed
Bug 543154
Opened 14 years ago
Closed 13 years ago
Uninitialised value use in nsNativeThemeWin::ClassicDrawWidgetBackground
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: jseward, Assigned: jimm)
Details
Attachments
(1 file, 1 obsolete file)
674 bytes,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
It appears that nsNativeThemeWin::ClassicGetThemePartAndState ( nsIFrame* aFrame, PRUint8 aWidgetType, PRInt32& aPart, PRInt32& aState, PRBool& aFocused ) in (m-c)/widget/src/windows/nsNativeThemeWin.cpp does not always write a value to 'aFocused'. This leads to an uninitialised value use in the caller of said routine, namely nsNativeThemeWin::ClassicDrawWidgetBackground, viz: Conditional jump or move depends on uninitialised value(s) at 0x10E22655: nsNativeThemeWin::ClassicDrawWidgetBackground (nsnativethemewin.cpp:2584) by 0x10E1F02A: nsNativeThemeWin::DrawWidgetBackground (nsnativethemewin.cpp:988) by 0x10600299: nsCSSRendering::PaintBackgroundWithSC (nscssrendering.cpp:2088) by 0x105FE9CF: nsCSSRendering::PaintBackground (nscssrendering.cpp:1416) by 0x106B2A78: nsDisplayBackground::Paint (nsdisplaylist.cpp:711) by 0x106B20A8: nsDisplayList::Paint (nsdisplaylist.cpp:415) by 0x106B3959: nsDisplayWrapList::Paint (nsdisplaylist.cpp:999) by 0x106B441B: nsDisplayClip::Paint (nsdisplaylist.cpp:1195) by 0x106B20A8: nsDisplayList::Paint (nsdisplaylist.cpp:415) by 0x106DA9A8: nsLayoutUtils::PaintFrame (nslayoututils.cpp:1195) by 0x1035E23A: PresShell::Paint (nspresshell.cpp:5776) by 0x104757E0: nsViewManager::RenderViews (nsviewmanager.cpp:497) Uninitialised value was created by a stack allocation at 0x10E22361: nsNativeThemeWin::ClassicDrawWidgetBackground (nsnativethemewin.cpp:2521) Analysis is as follows. All line numbers are in nsNativeThemeWin.cpp. In nsNativeThemeWin::ClassicDrawWidgetBackground we have 2522: PRInt32 part, state; 2523: PRBool focused; 2524: nsresult rv; 2525: rv = ClassicGetThemePartAndState(aFrame, aWidgetType, part, state, focused); Apparently it is intended to convert 'aWidgetType' (and possibly 'aFrame') into values for 'part', 'state' and 'focused' by calling ClassicGetThemePartAndState. Later in this same function we have: 2522: switch (aWidgetType) { [...] 2565: // Draw controls supported by DrawFrameControl 2566: case NS_THEME_CHECKBOX: 2567: case NS_THEME_RADIO: 2568: case NS_THEME_SCROLLBAR_BUTTON_UP: 2569: case NS_THEME_SCROLLBAR_BUTTON_DOWN: 2570: case NS_THEME_SCROLLBAR_BUTTON_LEFT: 2571: case NS_THEME_SCROLLBAR_BUTTON_RIGHT: 2572: case NS_THEME_SPINNER_UP_BUTTON: 2573: case NS_THEME_SPINNER_DOWN_BUTTON: 2574: case NS_THEME_DROPDOWN_BUTTON: 2575: case NS_THEME_RESIZER: { 2576: PRInt32 oldTA; 2577: // setup DC to make DrawFrameControl draw correctly 2578: oldTA = ::SetTextAlign(hdc, TA_TOP | TA_LEFT | TA_NOUPDATECP); 2579: ::DrawFrameControl(hdc, &widgetRect, part, state); 2580: ::SetTextAlign(hdc, oldTA); 2581: 2582: // Draw focus rectangles for HTML checkboxes and radio buttons 2583: // XXX it'd be nice to draw these outside of the frame 2584: if (focused && (aWidgetType == NS_THEME_CHECKBOX || aWidgetType == NS_THEME_RADIO)) { The error occurs at 2584, so the culprit must be 'focused' or 'aWidgetType'. It can't be the latter since V would have complained at 2522, so it must be the former. From inspection of ClassicGetThemePartAndState it's clear that refparam 'aFocused' is unset on most of the paths through. AFAICS it only gets set for aWidgetType being on of NS_THEME_BUTTON NS_THEME_CHECKBOX: NS_THEME_RADIO: That means, of the cases shown above at lines 2566 - 2575, when aWidgetType has any of these values NS_THEME_SCROLLBAR_BUTTON_UP NS_THEME_SCROLLBAR_BUTTON_DOWN NS_THEME_SCROLLBAR_BUTTON_LEFT NS_THEME_SCROLLBAR_BUTTON_RIGHT NS_THEME_SPINNER_UP_BUTTON NS_THEME_SPINNER_DOWN_BUTTON NS_THEME_DROPDOWN_BUTTON NS_THEME_RESIZER: then 'focused' is used uninitialised. I'm wondering if this is fallout from https://bugzilla.mozilla.org/show_bug.cgi?id=474878, but I'm far from certain.
![]() |
||
Comment 1•14 years ago
|
||
Nah, it looks like the bogus code dates back to at least Firefox 3.0. See http://mxr.mozilla.org/firefox/source/widget/src/windows/nsNativeThemeWin.cpp which has the same issues. In fact, it seems to date back to this checkin in CVS: 3.18 <timeless@mozdev.org> 2005-08-20 00:11 Bug 172751 nsITheme support for Windows 9x/NT/2000 patch by tim@prismelite.com r=hyatt sr=roc+moz
Reporter | ||
Comment 2•13 years ago
|
||
![]() |
Assignee | |
Comment 3•13 years ago
|
||
one line patch to fix an uninitialized variable in theme code.
Attachment #492353 -
Attachment is obsolete: true
Attachment #534852 -
Flags: review?(roc)
Comment on attachment 534852 [details] [diff] [review] fix Review of attachment 534852 [details] [diff] [review]: -----------------------------------------------------------------
Attachment #534852 -
Flags: review?(roc) → review+
![]() |
Assignee | |
Updated•13 years ago
|
Keywords: checkin-needed
Comment 5•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/4b1200504a4c
Assignee: nobody → jmathies
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in
before you can comment on or make changes to this bug.
Description
•