The default bug view has changed. See this FAQ.

Uninitialised value use in nsNativeThemeWin::ClassicDrawWidgetBackground

RESOLVED FIXED in mozilla7

Status

()

Core
Widget: Win32
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: jseward, Assigned: jimm)

Tracking

Other Branch
mozilla7
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
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

6 years ago
Created attachment 492353 [details] [diff] [review]
a fix
(Assignee)

Comment 3

6 years ago
Created attachment 534852 [details] [diff] [review]
fix

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

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/4b1200504a4c
Assignee: nobody → jmathies
Status: NEW → RESOLVED
Last Resolved: 6 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.