543154 - Uninitialised value use in nsNativeThemeWin::ClassicDrawWidgetBackground

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Julian Seward [:jseward]]

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

[Boris Zbarsky [:bzbarsky]]

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 <[e-mail]> 2005-08-20 00:11
Bug 172751 nsITheme support for Windows 9x/NT/2000
patch by [e-mail] r=hyatt sr=roc+moz

Comment 2

[Julian Seward [:jseward]]

Attachment: a fix

Comment 3

[Jim Mathies [:jimm]]

Attachment: fix

one line patch to fix an uninitialized variable in theme code.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Review of attachment 534852 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 5

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/4b1200504a4c