Closed Bug 866959 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Graphics: ImageLib, defect)

x86_64
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: gkw, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, regression, valgrind)

Attachments

(2 files)

Attached file 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.
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.
Flags: needinfo?(joe)
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.
Group: core-security
Attached patch patchSplinter Review
Bug 854287 added mHasBeenDecoded but only initialized it in the imgStatusTracker(Image* aImage) constructor but not the imgStatusTracker(const imgStatusTracker& aOther) constructor.
Assignee: nobody → tnikkel
Attachment #743453 - Flags: review?(seth)
Flags: needinfo?(joe)
Blocks: 854287
Comment on attachment 743453 [details] [diff] [review]
patch

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

Looks good. Thanks Timothy!
Attachment #743453 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/e0f1b2ba992d

Can we get a test for this?
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.