866959 - Valgrind detects a Conditional jump or move depends on uninitialised value(s) error with mozilla::image::RasterImage on the stack

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

Valgrind detects a Conditional jump or move depends on uninitialised value(s) error with mozilla::image::RasterImage on the stack, see attached snippet which comes from:

https://tbpl.mozilla.org/php/getParsedLog.php?id=22356838&tree=Mozilla-Central&full=1

Guessing Core: ImageLib, please change component if necessary.

Regression window:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3ada6a2fd0c6&tochange=64d6d002e888

s-s because conditional jumps can be bad, locking pending developer confirmation.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Valgrind points to:

==12769==  Uninitialised value was created by a heap allocation
==12769==    at 0x4C28A49: malloc (vg_replace_malloc.c:270)
==12769==    by 0x760D005: moz_xmalloc (mozalloc.cpp:54)
==12769==    by 0x8209756: imgStatusTracker::CloneForRecording() (mozalloc.h:201)
==12769==    by 0x81F17E6: mozilla::image::RasterImage::InitDecoder(bool, bool) (RasterImage.h:401)
==12769==    by 0x81F1EDB: mozilla::image::RasterImage::Init(char const*, unsigned int) (RasterImage.cpp:517)
==12769==    by 0x81E7D47: mozilla::image::ImageFactory::CreateRasterImage(nsIRequest*, imgStatusTracker*, nsCString const&, nsIURI*, unsigned int, unsigned int) (ImageFactory.cpp:189)
==12769==    by 0x81E7FF3: mozilla::image::ImageFactory::CreateImage(nsIRequest*, imgStatusTracker*, nsCString const&, nsIURI*, bool, unsigned int) (ImageFactory.cpp:106)

/snip

cc'ing some folks who might know what's going on.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't think this is s-s.  The uninitialized value is used to determine whether or not to do invalidations, so while there's likely a rendering correctness problem here, there should be no security issues.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Suppression landed in:

https://hg.mozilla.org/mozilla-central/rev/1eb382609c2d

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Bug 854287 added mHasBeenDecoded but only initialized it in the imgStatusTracker(Image* aImage) constructor but not the imgStatusTracker(const imgStatusTracker& aOther) constructor.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 743453 [details] [diff] [review]
patch

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

Looks good. Thanks Timothy!

Comment 6

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f1b2ba992d

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0f1b2ba992d

Can we get a test for this?

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Suppression removed in:

https://hg.mozilla.org/mozilla-central/rev/d44cfdc9ec2e