543864 - Uninitialised value use in nsLayoutUtils::DrawSingleImage (nsLayoutUtils.cpp:3068)

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Julian Seward [:jseward]]

Bizarrely enough, Valgrind is complaining about the assertion right at
the start of nsLayoutUtils::DrawSingleImage (nsLayoutUtils.cpp:3058)

  nsIntSize imageSize;
  aImage->GetWidth(&imageSize.width);
  aImage->GetHeight(&imageSize.height);
  NS_ENSURE_TRUE(imageSize.width > 0 && imageSize.height > 0, NS_ERROR_FAILURE);

It's complaining about the condition inside NS_ENSURE_TRUE.  My guess
is either ->GetWidth() or ->GetHeight() occasionally does not write
anything to its argument.  This is lent credence by changing the
fragment thusly:

  nsIntSize imageSize;
  aImage->GetWidth(&imageSize.width);
  printf("XXXXXXXXXXXXw %d\n", imageSize.width);
  aImage->GetHeight(&imageSize.height);
  printf("XXXXXXXXXXXXh %d\n", imageSize.height);
  NS_ENSURE_TRUE(imageSize.width > 0 && imageSize.height > 0, NS_ERROR_FAILURE);

Most of the time it prints unexciting numbers for the width and height
(eg, zero, 16, 32, etc).  But occasionally it does this

  XXXXXXXXXXXXw 32640104
  XXXXXXXXXXXXh 0
  XXXXXXXXXXXXw 32636312
  XXXXXXXXXXXXh 0

  XXXXXXXXXXXXw 32761000
  XXXXXXXXXXXXh 0
  XXXXXXXXXXXXw 32760376
  XXXXXXXXXXXXh 0

which have the look of bogosity about them.

Valgrind's barfage is below.

HOW TO REPRO: TM trunk of a couple of hours back, x86-64 linux,
mozconfig thusly

  . $topsrcdir/browser/config/mozconfig
  mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/ff-opt
  ac_add_options --enable-tests
  ac_add_options --enable-optimize="-g -O -freorder-blocks"
  ac_add_options --disable-jemalloc

Spark up.  Minefield comes up with the default URL
http://start.ubuntu.com/9.10

Place mouse cursor in URL bar, at end of URL, and start backspacing.
The Awesome bar appears and disappears.  When you erase the last
character (viz, the "h") the error happens.  If not, type a couple
chars and backspace over them.

-------

There's another thing as well.  I don't know if it's related, but ..
when gcc-4.4 compiles this source file (nsLayoutUtils.cpp) it warns
that

/home/sewardj/MOZ/WD_Feb02/layout/base/nsLayoutUtils.cpp: 
   In static member function ‘static nscoord
   nsLayoutUtils::ComputeWidthValue(nsIRenderingContext*, nsIFrame*,
   nscoord, nscoord, nscoord, const nsStyleCoord&)’:
   /home/sewardj/MOZ/WD_Feb02/layout/base/nsLayoutUtils.cpp:2221:
      warning: ‘result’ may be used uninitialized in this function

/home/sewardj/MOZ/WD_Feb02/layout/base/nsLayoutUtils.cpp:
   In static member function ‘static nsSize
   nsLayoutUtils::ComputeSizeWithIntrinsicDimensions(nsIRenderingContext*,
   nsIFrame*, const nsIFrame::IntrinsicSize&, nsSize, nsSize, nsSize,
   nsSize, nsSize)’:
   /home/sewardj/MOZ/WD_Feb02/layout/base/nsLayoutUtils.cpp:2338:
      warning: ‘width’ may be used uninitialized in this function
   /home/sewardj/MOZ/WD_Feb02/layout/base/nsLayoutUtils.cpp:2338:
      warning: ‘height’ may be used uninitialized in this function

Note, these source locations are different from what Valgrind
mentions.  So these might be unrelated.

-------

Anyway, V's output is as follows.  (Actually there were two of
these, one for each of the comparisons in 
"imageSize.width > 0 && imageSize.height > 0".  I hunted down
and checked the machine code.)


Conditional jump or move depends on uninitialised value(s)
   at 0x573E519: nsLayoutUtils::DrawSingleImage(nsIRenderingContext*, imgIContainer*, gfxPattern::GraphicsFilter, nsRect const&, nsRect const&, unsigned int, nsRect const*) (nsLayoutUtils.cpp:3068)
   by 0x587D781: nsImageBoxFrame::PaintImage(nsIRenderingContext&, nsRect const&, nsPoint, unsigned int) (nsImageBoxFrame.cpp:388)
   by 0x587D7FE: nsDisplayXULImage::Paint(nsDisplayListBuilder*, nsIRenderingContext*) (nsImageBoxFrame.cpp:339)
   by 0x5729B18: nsDisplayList::Paint(nsDisplayListBuilder*, nsIRenderingContext*) const (nsDisplayList.cpp:415)
   by 0x5729B38: nsDisplayWrapList::Paint(nsDisplayListBuilder*, nsIRenderingContext*) (nsDisplayList.cpp:999)
   by 0x5729CBC: nsDisplayClip::Paint(nsDisplayListBuilder*, nsIRenderingContext*) (nsDisplayList.cpp:1195)
   by 0x5729B18: nsDisplayList::Paint(nsDisplayListBuilder*, nsIRenderingContext*) const (nsDisplayList.cpp:415)
   by 0x5729B38: nsDisplayWrapList::Paint(nsDisplayListBuilder*, nsIRenderingContext*) (nsDisplayList.cpp:999)
   by 0x5729CBC: nsDisplayClip::Paint(nsDisplayListBuilder*, nsIRenderingContext*) (nsDisplayList.cpp:1195)
   by 0x5729B18: nsDisplayList::Paint(nsDisplayListBuilder*, nsIRenderingContext*) const (nsDisplayList.cpp:415)
   by 0x573BF83: nsLayoutUtils::PaintFrame(nsIRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) (nsLayoutUtils.cpp:1195)
   by 0x5747AC2: PresShell::Paint(nsIView*, nsIRenderingContext*, nsRegion const&) (nsPresShell.cpp:5779)
 Uninitialised value was created by a stack allocation
   at 0x573E4AA: nsLayoutUtils::DrawSingleImage(nsIRenderingContext*, imgIContainer*, gfxPattern::GraphicsFilter, nsRect const&, nsRect const&, unsigned int, nsRect const*) (nsLayoutUtils.cpp:3064)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Seems rather similar to bug 541028.

We should probably either audit all callers or change the behavior of these functions on error return.

Comment 2

[Brandon Sterne (:bsterne)]

Having some difficulty deciding on the severity here.  imageSize is used in function args several times after the UMR, so it's possible a determined attacker could exploit this.  On the other hand the bogus GetWidth() and GetHeight() returns seem somewhat hard to hit.  Feel free to raise the severity if you feel it's appropriate.  We should fix this, though.

Comment 3

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

I vote for having those methods return 0 for uninitialized images.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Sounds reasonable to me.

Comment 5

[Jesse Ruderman]

[sg:low?] to match bug 541028 and because this is an unlikely-useful-to-attackers UMR.

Comment 6

[Julian Seward [:jseward]]

(Repro comment) This can be caused by going to
http://www.tamburin-stuttgart.de, and getting the URL into the
auto-dropdown menu of the Awesome bar.  I noticed that this is the
only URL in there that doesn't have one of those little icon images,
so that's perhaps related.

Comment 7

[Brandon Sterne (:bsterne)]

Roc, can you find the appropriate person to fix this please?

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

When this is ready to land, please also make sure to request approval on bug 541028 as well.

Comment 9

[Julian Seward [:jseward]]

Still visible on Linux using URL in comment #6, but not detectable
via any mochitest, curiously.  Very visible when running mochitests
on valgrind on OSX 10.5 (32-bit).

Comment 10

[Julian Seward [:jseward]]

(In reply to comment #3)
> I vote for having those methods return 0 for uninitialized images.

I'd give it a go, but I don't know how to find the complete set of
methods involved.  I lost the trail in a swamp of IDLage and
inheritance, at this point

  /**
   * The width of the container rectangle.
   */
  /* readonly attribute PRInt32 width; */
  NS_SCRIPTABLE NS_IMETHOD GetWidth(PRInt32 *aWidth) = 0;

  /**
   * The height of the container rectangle.
   */
  /* readonly attribute PRInt32 height; */
  NS_SCRIPTABLE NS_IMETHOD GetHeight(PRInt32 *aHeight) = 0;

in $objdir/modules/libpr0n/public/_xpidlgen/imgIContainer.h


Alternatively I can patch this particular call site, in the style of
bug 541028.  But that strikes me as rather a whack-a-mole approach.


It'd be nice to get this fixed.  It causes valgrind to spit out more
than 30 complaints on a startup of mochitests on OSX 10.5 (32-bit),
before even the first test is run.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Bobby, what's our current set of imgIContainer implementations?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Bobby, what's our current set of imgIContainer implementations?

Comment 13

[Bobby Holley (:bholley)]

(In reply to comment #12)
> Bobby, what's our current set of imgIContainer implementations?

Should be just mozilla::Imagelib::Image (a superclass).  Right now the only thing that inherits it is mozilla::Imagelib::RasterImage, but we'll soon have VectorImage too when bug 276431 lands (any day now).

Comment 14

[Julian Seward [:jseward]]

Attachment: patch, as per comments 3 and 4

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this still the approach we want now that SVG-as-image landed?

Comment 16

[Daniel Holbert [:dholbert]]

Technically, callers should be checking the return value of GetWidth() & GetHeight() before they read its outparam.  This bug's fix still seems reasonable, though, as a defense-in-depth sort of thing.

We need the same fix in VectorImage.cpp, to "fully" fix this.  We also should probably mention in the .idl file that errors will trigger an exception (a NS_ERROR_FAILURE nsresult being returned) in addition to the 0 length that's returned-by-reference.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: followup: mention exception in .idl, & fix VectorImage

(In reply to comment #16)
> We need the same fix in VectorImage.cpp, to "fully" fix this.  We also should
> probably mention in the .idl file that errors will trigger an exception (a
> NS_ERROR_FAILURE nsresult being returned) in addition to the 0 length that's
> returned-by-reference.

Addressed both of these points in this followup patch.

Comment 18

[Daniel Holbert [:dholbert]]

Per beltzner's post to m.d.planning [1], the tree is now restricted to b7 blockers, for the next few days at least.

I'll plan on checking in this bug's patches after that (for beta 8).

[1] http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/f972f7db45021bb2#

Comment 19

[Daniel Holbert [:dholbert]]

Landed:
patch 1: http://hg.mozilla.org/mozilla-central/rev/6175490c92ad
patch 2: http://hg.mozilla.org/mozilla-central/rev/72a597f084e2

I moved patch 2's idl-comment-tweak into patch 1 before landing, so that if we backport this to 1.9.* branches, we can just take patch 1 and be in a more consistent state (comment-wise) with branches vs. trunk.  (patch 2's VectorImage changes aren't applicable on branches).

Comment 20

[Daniel Holbert [:dholbert]]

So I'd thought this was intended for 1.9.1 & 1.9.2, since status1.9.1 & 1.9.2 = "wanted" right now.  But I don't think this affects 1.9.1/1.9.2, actually.

This *was* a problem on mozilla-central trunk, because we had a return-early "error" case where we didn't set our outparam to anything:

388 RasterImage::GetWidth(PRInt32 *aWidth)
389 {
390   NS_ENSURE_ARG_POINTER(aWidth);
391 
392   if (mError)
393     return NS_ERROR_FAILURE;

However, in 1.9.1 and 1.9.2, there's no |mError| variable and no early-return case, as shown below:
174 NS_IMETHODIMP imgContainer::GetWidth(PRInt32 *aWidth)
175 {
176   NS_ENSURE_ARG_POINTER(aWidth);
177 
178   *aWidth = mSize.width;
179   return NS_OK;
http://mxr.mozilla.org/mozilla1.9.2/source/modules/libpr0n/src/imgContainer.cpp#172
(note that RasterImage is called imgContainer in 1.9.2 and earlier)

And mSize is initialized to 0,0 in the imgContainer constructor (for both 1.9.1 and 1.9.2), so even if we were in an error state, we'd have an initialized value in *aWidth.

So: I think this should have status1.9.2 & status1.9.1 = "unaffected".

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to comment #20)
> However, in 1.9.1 and 1.9.2, there's no |mError| variable and no early-return
> case

(Minor clarification: technically, there *is* one early-return case on 1.9.1 & 1.9.2 inside the NS_ENSURE_ARG_POINTER macro.  But this bug isn't applicable for that -- if we ever return early from that statement, we'll have a null outparam-pointer, so there'd be no uninitialized-variable issue).