798989 - Uninitialised value use in gfxUtils::GetYCbCrToRGBDestFormatAndSize (aSuggestedFormat)

Status: NEW

(Core :: Graphics: Layers, defect)

Description

[Matthew Gregan [:kinetik]]

Conditional jump or move depends on uninitialised value(s)
   at 0x6455231: gfxUtils::GetYCbCrToRGBDestFormatAndSize(layers::PlanarYCbCrImage::Data const&, gfxASurface::gfxImageFormat&, nsIntSize&) (gfxUtils.cpp:647)
   by 0x6477F26: layers::PlanarYCbCrImage::GetAsSurface() (ImageContainer.cpp:483)
   by 0x64788A5: layers::ImageContainer::LockCurrentAsSurface(nsIntSize*, layers::Image**) (ImageContainer.cpp:287)
   by 0x647328B: layers::BasicImageLayer::GetAndPaintCurrentImage(gfxContext*, float, layers::Layer*) (ImageContainer.h:560)
   by 0x64735C2: layers::BasicImageLayer::Paint(gfxContext*, layers::Layer*) (BasicImageLayer.cpp:73)
   by 0x6473FB9: layers::BasicShadowableImageLayer::Paint(gfxContext*, layers::Layer*) (BasicImageLayer.cpp:248)
   by 0x646B7B1: layers::BasicLayerManager::PaintSelfOrChildren(layers::PaintContext&, gfxContext*) (BasicLayerManager.cpp:815)

Conditional jump or move depends on uninitialised value(s)
   at 0x6455236: gfxUtils::GetYCbCrToRGBDestFormatAndSize(layers::PlanarYCbCrImage::Data const&, gfxASurface::gfxImageFormat&, nsIntSize&) (gfxUtils.cpp:670)
   by 0x6477F26: layers::PlanarYCbCrImage::GetAsSurface() (ImageContainer.cpp:483)
   by 0x64788A5: layers::ImageContainer::LockCurrentAsSurface(nsIntSize*, layers::Image**) (ImageContainer.cpp:287)
   by 0x647328B: layers::BasicImageLayer::GetAndPaintCurrentImage(gfxContext*, float, layers::Layer*) (ImageContainer.h:560)
   by 0x64735C2: layers::BasicImageLayer::Paint(gfxContext*, layers::Layer*) (BasicImageLayer.cpp:73)
   by 0x6473FB9: layers::BasicShadowableImageLayer::Paint(gfxContext*, layers::Layer*) (BasicImageLayer.cpp:248)
   by 0x646B7B1: layers::BasicLayerManager::PaintSelfOrChildren(layers::PaintContext&, gfxContext*) (BasicLayerManager.cpp:815)

This is referring to mOffscreenSurface in PlanarYCbCrImage, which is initialised via SetOffscreenFormat.  SetOffscreenFormat is only ever called from the subclass BasicPlanarYCbCrImage.  This instance of PlanarYCbCrImage was created by ImageFactory::Create.

Comment 1

[Oleg Romashin (:romaxa)]

Sounds like ImageFactory::Create should initialize mOffscreenSurface

Comment 2

[ISHIKAWA, Chiaki]

Hi, I think there are other variable(s) that may require initialization.

Title: Uninitialized value usage in mozilla/gfx/thebes/gfxUtils.cpp

Found by valgrind run. See bug 803816 about mozmill run of TB.

comm-central thunderbird.
Source version:
$ hg identify
1016cef82fd8+ tip
ishikawa@debian-vm:~/TB-NEW/TB-3HG/new-src$ cd mozilla
ishikawa@debian-vm:~/TB-NEW/TB-3HG/new-src/mozilla$ hg identify
a517f7ea5bef+ tip

There are a few cases quoted below and more cases of warnings that seem to
be caused uninitialized image data structure. (Caused by
allocation in CreateImage() [and possibly the lower level routines that are called under it.]) 

CASE I:

valgrind log

15887:==27214== Conditional jump or move depends on uninitialised value(s)
15888-==27214==    at 0x606F9F1: gfxUtils::GetYCbCrToRGBDestFormatAndSize(mozilla::layers::PlanarYCbCrImage::Data const&, gfxASurface::gfxImageFormat&, nsIntSize&) (gfxUtils.cpp:647)
15889-==27214==    by 0x473155B: ??? (in /TB-NEW/TB-3HG/objdir-tb3/mozilla/nsprpub/pr/src/libnspr4.so)
15890:==27214==  Uninitialised value was created by a heap allocation
15891-==27214==    at 0x40271C4: malloc (vg_replace_malloc.c:270)
15892-==27214==    by 0x4041E10: moz_xmalloc (mozalloc.cpp:54)
15893-==27214==    by 0x6098170: mozilla::layers::ImageFactory::CreateImage(mozilla::ImageFormat const*, unsigned int, nsIntSize const&, mozilla::layers::BufferRecycleBin*) (mozalloc.h:200)
15894-==27214==    by 0x6097AE1: mozilla::layers::ImageContainer::CreateImage(mozilla::ImageFormat const*, unsigned int) (ImageContainer.cpp:147)
15895-==27214==    by 0x55AADD7: VideoData::Create(nsVideoInfo&, mozilla::layers::ImageContainer*, long long, long long, long long, VideoData::YCbCrBuffer const&, bool, long long, nsIntRect) (nsBuiltinDecoderReader.cpp:209)
15896-==27214==    by 0x55BD54B: nsOggReader::DecodeTheora(ogg_packet*, long long) (nsOggReader.cpp:651)

Observation:


mozilla/gfx/thebes/gfxUtils.cpp

  632	gfxUtils::GetYCbCrToRGBDestFormatAndSize(const PlanarYCbCrImage::Data& aData,
  633	                                         gfxASurface::gfxImageFormat& aSuggestedFormat,
  634	                                         gfxIntSize& aSuggestedSize)
  635	{
  636	  gfx::YUVType yuvtype =
  637	    gfx::TypeFromSize(aData.mYSize.width,
  638	                      aData.mYSize.height,
  639	                      aData.mCbCrSize.width,
  640	                      aData.mCbCrSize.height);
  641	
  642	  // 'prescale' is true if the scaling is to be done as part of the
  643	  // YCbCr to RGB conversion rather than on the RGB data when rendered.
  644	  bool prescale = aSuggestedSize.width > 0 && aSuggestedSize.height > 0 &&
  645	                    aSuggestedSize != aData.mPicSize;
  646	
  647	  if (aSuggestedFormat == gfxASurface::ImageFormatRGB16_565) {

  
  ImageContainer.cpp
  143	ImageContainer::CreateImage(const ImageFormat *aFormats,
  144	                            uint32_t aNumFormats)
  145	{
  146	  ReentrantMonitorAutoEnter mon(mReentrantMonitor);
  147	  return mImageFactory->CreateImage(aFormats, aNumFormats, mScaleHint, mRecycleBin);
  148	}
  149	




CASE II

15911:==27214== Conditional jump or move depends on uninitialised value(s)
15912-==27214==    at 0x606F9F6: gfxUtils::GetYCbCrToRGBDestFormatAndSize(mozilla::layers::PlanarYCbCrImage::Data const&, gfxASurface::gfxImageFormat&, nsIntSize&) (gfxUtils.cpp:670)
15913-==27214==    by 0x473155B: ??? (in /TB-NEW/TB-3HG/objdir-tb3/mozilla/nsprpub/pr/src/libnspr4.so)
15914:==27214==  Uninitialised value was created by a heap allocation
15915-==27214==    at 0x40271C4: malloc (vg_replace_malloc.c:270)
15916-==27214==    by 0x4041E10: moz_xmalloc (mozalloc.cpp:54)
15917-==27214==    by 0x6098170: mozilla::layers::ImageFactory::CreateImage(mozilla::ImageFormat const*, unsigned int, nsIntSize const&, mozilla::layers::BufferRecycleBin*) (mozalloc.h:200)
15918-==27214==    by 0x6097AE1: mozilla::layers::ImageContainer::CreateImage(mozilla::ImageFormat const*, unsigned int) (ImageContainer.cpp:147)
15919-==27214==    by 0x55AADD7: VideoData::Create(nsVideoInfo&, mozilla::layers::ImageContainer*, long long, long long, long long, VideoData::YCbCrBuffer const&, bool, long long, nsIntRect) (nsBuiltinDecoderReader.cpp:209)
15920-==27214==    by 0x55BD54B: nsOggReader::DecodeTheora(ogg_packet*, long long) (nsOggReader.cpp:651)
--


     This is more difficult to figure out.
     If aSuggestedFormat is not defined on line 670, it ought to
     have raised a warning on line 647 already as in case I.
     Am I missing something?
 
  647	  if (aSuggestedFormat == gfxASurface::ImageFormatRGB16_565) {
  648	#if defined(HAVE_YCBCR_TO_RGB565)
  649	    if (prescale &&
  650	        !gfx::IsScaleYCbCrToRGB565Fast(aData.mPicX,
  651	                                       aData.mPicY,
  652	                                       aData.mPicSize.width,
  653	                                       aData.mPicSize.height,
  654	                                       aSuggestedSize.width,
  655	                                       aSuggestedSize.height,
  656	                                       yuvtype,
  657	                                       gfx::FILTER_BILINEAR) &&
  658	        gfx::IsConvertYCbCrToRGB565Fast(aData.mPicX,
  659	                                        aData.mPicY,
  660	                                        aData.mPicSize.width,
  661	                                        aData.mPicSize.height,
  662	                                        yuvtype)) {
  663	      prescale = false;
  664	    }
  665	#else
  666	    // yuv2rgb16 function not available
  667	    aSuggestedFormat = gfxASurface::ImageFormatRGB24;
  668	#endif
  669	  }
  670	  else if (aSuggestedFormat != gfxASurface::ImageFormatRGB24) { 



CASE III:

15935:==27214== Conditional jump or move depends on uninitialised value(s)
15936-==27214==    at 0x604D7B6: gfxImageSurface::ComputeStride(nsIntSize const&, gfxASurface::gfxImageFormat) (gfxImageSurface.cpp:157)
15937-==27214==    by 0x604D8A0: gfxImageSurface::gfxImageSurface(nsIntSize const&, gfxASurface::gfxImageFormat, bool) (gfxImageSurface.h:100)
15938-==27214==    by 0x609852E: mozilla::layers::PlanarYCbCrImage::GetAsSurface() (ImageContainer.cpp:491)
15939-==27214==    by 0x6098A37: mozilla::layers::ImageContainer::LockCurrentAsSurface(nsIntSize*, mozilla::layers::Image**) (ImageContainer.cpp:287)
15940-==27214==    by 0x27F: ???
15941:==27214==  Uninitialised value was created by a heap allocation
15942-==27214==    at 0x40271C4: malloc (vg_replace_malloc.c:270)
15943-==27214==    by 0x4041E10: moz_xmalloc (mozalloc.cpp:54)
15944-==27214==    by 0x6098170: mozilla::layers::ImageFactory::CreateImage(mozilla::ImageFormat const*, unsigned int, nsIntSize const&, mozilla::layers::BufferRecycleBin*) (mozalloc.h:200)
15945-==27214==    by 0x6097AE1: mozilla::layers::ImageContainer::CreateImage(mozilla::ImageFormat const*, unsigned int) (ImageContainer.cpp:147)
15946-==27214==    by 0x55AADD7: VideoData::Create(nsVideoInfo&, mozilla::layers::ImageContainer*, long long, long long, long long, VideoData::YCbCrBuffer const&, bool, long long, nsIntRect) (nsBuiltinDecoderReader.cpp:209)
15947-==27214==    by 0x55BD54B: nsOggReader::DecodeTheora(ogg_packet*, long long) (nsOggReader.cpp:651)

The image data dynamically allocated is not properly initialized and
eventually, its uninitialized data is referenced in ComputeStride.

  152	/*static*/ long
  153	gfxImageSurface::ComputeStride(const gfxIntSize& aSize, gfxImageFormat aFormat)
  154	{
  155	    long stride;
  156	
  157	    if (aFormat == ImageFormatARGB32)
  158	        stride = aSize.width * 4;
  159	    else if (aFormat == ImageFormatRGB24)

This is caused by the failure to initialize data allocated by the same
routine, CreateImage() in the case I above. So I am reporting this
problem in this same bugzilla entry.

  ImageContainer.cpp
  143	ImageContainer::CreateImage(const ImageFormat *aFormats,
  144	                            uint32_t aNumFormats)


CASE IV

==27214== Conditional jump or move depends on uninitialised value(s)
==27214==    at 0x604D7BB: gfxImageSurface::ComputeStride(nsIntSize const&, gfxASurface::gfxImageFormat) (gfxImageSurface.cpp:159)
==27214==    by 0x604D8A0: gfxImageSurface::gfxImageSurface(nsIntSize const&, gfxASurface::gfxImageFormat, bool) (gfxImageSurface.h:100)
==27214==    by 0x609852E: mozilla::layers::PlanarYCbCrImage::GetAsSurface() (ImageContainer.cpp:491)
==27214==    by 0x6098A37: mozilla::layers::ImageContainer::LockCurrentAsSurface(nsIntSize*, mozilla::layers::Image**) (ImageContainer.cpp:287)
==27214==    by 0x27F: ???
==27214==  Uninitialised value was created by a heap allocation
==27214==    at 0x40271C4: malloc (vg_replace_malloc.c:270)
==27214==    by 0x4041E10: moz_xmalloc (mozalloc.cpp:54)
==27214==    by 0x6098170: mozilla::layers::ImageFactory::CreateImage(mozilla::ImageFormat const*, unsigned int, nsIntSize const&, mozilla::layers::BufferRecycleBin*) (mozalloc.h:200)
==27214==    by 0x6097AE1: mozilla::layers::ImageContainer::CreateImage(mozilla::ImageFormat const*, unsigned int) (ImageContainer.cpp:147)
==27214==    by 0x55AADD7: VideoData::Create(nsVideoInfo&, mozilla::layers::ImageContainer*, long long, long long, long long, VideoData::YCbCrBuffer const&, bool, long long, nsIntRect) (nsBuiltinDecoderReader.cpp:209)
==27214==    by 0x55BD54B: nsOggReader::DecodeTheora(ogg_packet*, long long) (nsOggReader.cpp:651)
==27214==    by 0x55BF1D5: nsOggReader::DecodeVideoFrame(bool&, long long) (nsOggReader.cpp:696)
==27214==    by 0x55AA149: nsBuiltinDecoderReader::DecodeVideoFrame() (nsBuiltinDecoderReader.h:523)
==27214==    by 0x55AA455: nsBuiltinDecoderReader::FindStartTime(long long&) (nsBuiltinDecoderReader.h:513)
==27214==    by 0x55A453A: nsBuiltinDecoderStateMachine::FindStartTime() (nsBuiltinDecoderStateMachine.cpp:2366)
==27214==    by 0x55A659E: nsBuiltinDecoderStateMachine::DecodeMetadata() (nsBuiltinDecoderStateMachine.cpp:1755)
==27214==    by 0x55AA0E4: nsBuiltinDecoderStateMachine::DecodeThreadRun() (nsBuiltinDecoderStateMachine.cpp:490)
==27214==    by 0x426723D: clone (clone.S:130)


     Note the line number in gfxImageSurface.cpp: 159. Case III has
     157.
     Can it be that there are cases where aFormat is defined, but
     aSize is undefined?

CASE nnn:

There are many cases where the warning seems to be triggered by
non-initialized data created by CreateImage() routine (or
possibly further allocated by a lower routine that gets called below
it.) See the full log psted in bug 803816.


Right, ImageFactor::Create may be the better higher-level routine to initialize that, come to think of it, but I will leave the decision to people in the know.

It is nice to fix this since there are SO MANY warnings related to this problem in "make mozmill" run of TB.

TIA