Last Comment Bug 687982 - Innocuous uninitialized value in nsBMPDecoder
: Innocuous uninitialized value in nsBMPDecoder
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla11
Assigned To: Brian R. Bondy [:bbondy]
:
: Milan Sreckovic [:milan]
Mentors:
: 704512 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-20 13:33 PDT by Joe Drew (not getting mail)
Modified: 2012-01-18 16:29 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Uninitialized BMP value fix (2.77 KB, patch)
2011-11-14 13:40 PST, Brian R. Bondy [:bbondy]
joe: review+
Details | Diff | Splinter Review

Description Joe Drew (not getting mail) 2011-09-20 13:33:59 PDT
I was running Firefox through Valgrind and came across:

==54156== Conditional jump or move depends on uninitialised value(s)
==54156==    at 0x10748F204: mozilla::imagelib::nsBMPDecoder::WriteInternal(char const*, unsigned int) (nsBMPDecoder.cpp:382)
==54156==    by 0x107455880: mozilla::imagelib::Decoder::Write(char const*, unsigned int) (Decoder.cpp:123)
==54156==    by 0x107491D6A: mozilla::imagelib::nsICODecoder::WriteInternal(char const*, unsigned int) (nsICODecoder.cpp:400)
==54156==    by 0x107455880: mozilla::imagelib::Decoder::Write(char const*, unsigned int) (Decoder.cpp:123)
==54156==    by 0x107458A8D: mozilla::imagelib::RasterImage::WriteToDecoder(char const*, unsigned int) (RasterImage.cpp:2293)
==54156==    by 0x107458C8A: mozilla::imagelib::RasterImage::DecodeSomeData(unsigned int) (RasterImage.cpp:2625)
==54156==    by 0x107458EAD: mozilla::imagelib::imgDecodeWorker::Run() (RasterImage.cpp:2744)
==54156==    by 0x107459789: mozilla::imagelib::RasterImage::AddSourceData(char const*, unsigned int) (RasterImage.cpp:1281)
==54156==    by 0x1074598D4: mozilla::imagelib::RasterImage::WriteToRasterImage(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) (RasterImage.cpp:2831)
==54156==    by 0x1088ECB6A: nsPipeInputStream::ReadSegments(unsigned int (*)(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) (nsPipe3.cpp:799)
==54156==    by 0x10747E3CC: imgRequest::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) (imgRequest.cpp:1158)
==54156==    by 0x10746BB8B: ProxyListener::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) (imgLoader.cpp:2100)
==54156== 
==54156== Conditional jump or move depends on uninitialised value(s)
==54156==    at 0x10748F4D6: mozilla::imagelib::nsBMPDecoder::WriteInternal(char const*, unsigned int) (nsBMPDecoder.cpp:419)
==54156==    by 0x107455880: mozilla::imagelib::Decoder::Write(char const*, unsigned int) (Decoder.cpp:123)
==54156==    by 0x107491D6A: mozilla::imagelib::nsICODecoder::WriteInternal(char const*, unsigned int) (nsICODecoder.cpp:400)
==54156==    by 0x107455880: mozilla::imagelib::Decoder::Write(char const*, unsigned int) (Decoder.cpp:123)
==54156==    by 0x107458A8D: mozilla::imagelib::RasterImage::WriteToDecoder(char const*, unsigned int) (RasterImage.cpp:2293)
==54156==    by 0x107458C8A: mozilla::imagelib::RasterImage::DecodeSomeData(unsigned int) (RasterImage.cpp:2625)
==54156==    by 0x107458EAD: mozilla::imagelib::imgDecodeWorker::Run() (RasterImage.cpp:2744)
==54156==    by 0x107459789: mozilla::imagelib::RasterImage::AddSourceData(char const*, unsigned int) (RasterImage.cpp:1281)
==54156==    by 0x1074598D4: mozilla::imagelib::RasterImage::WriteToRasterImage(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) (RasterImage.cpp:2831)
==54156==    by 0x1088ECB6A: nsPipeInputStream::ReadSegments(unsigned int (*)(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) (nsPipe3.cpp:799)
==54156==    by 0x10747E3CC: imgRequest::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) (imgRequest.cpp:1158)
==54156==    by 0x10746BB8B: ProxyListener::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) (imgLoader.cpp:2100)
==54156==
Comment 1 Justin Lebar (not reading bugmail) 2011-09-20 14:02:48 PDT
I thought this was known and innocuous?
Comment 2 Brian R. Bondy [:bbondy] 2011-11-12 19:25:36 PST
I know this one is older but for bug 700930 it has the side effect icons as attachments.  I don't know why yet but I'm almost positive that the problem is with bpc getting a value of 3 instead of 4.

bpc = (mBFH.bihsize == OS2_BIH_LENGTH) ? 3 : 4; // OS/2 Bitmaps have no padding byte
   
Will fix soon and suggest aurora and beta pushes.

On a side note I setup valgrind and did get some uninitialized value errors but not one in nsBMPDecoder.  I think it depends on how much data is passed in each WriteInternal call.

*** This bug has been marked as a duplicate of bug 700930 ***
Comment 3 Brian R. Bondy [:bbondy] 2011-11-14 13:36:59 PST
It turns out that this bug is not related to what I thought was the cause of bug 700930.
Comment 4 Brian R. Bondy [:bbondy] 2011-11-14 13:40:56 PST
Created attachment 574405 [details] [diff] [review]
Uninitialized BMP value fix

The uninitialized variable is indeed innocuous and is only because we calculate the value of bpc before and after the header was read.

> bpc = (mBFH.bihsize == OS2_BIH_LENGTH) ? 3 : 4;

The bpc value (mBFH.bihsize) is only actually used when the header is is already read.
 
The patch also fixes up a couple comments to be on their own line and renames bpc to bytesPerColor since it's a better self explanatory name.
Comment 5 Brian R. Bondy [:bbondy] 2011-11-14 13:46:41 PST
"The bpc value (mBFH.bihsize) is only actually used when the header is is already read."

Should read:

The bpc value is only actually used when the header is is already read.
The uninitialized value which causes the valgrind warning is mBFH.bihsize.
Comment 6 Brian R. Bondy [:bbondy] 2011-11-14 17:27:58 PST
Thanks for the quick review!

I'm sure it's fine to push to m-i since it still passes all reftests, but I pushed to try first to be safe.

Results here:
https://tbpl.mozilla.org/?tree=Try&rev=145d69060885

Will push to m-i if that succeeds.
Comment 7 Brian R. Bondy [:bbondy] 2011-11-16 15:35:15 PST
This was pushed to m-i earlier today by the way:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d339796e649b
Comment 8 Marco Bonardo [::mak] 2011-11-17 03:00:16 PST
https://hg.mozilla.org/mozilla-central/rev/d339796e649b
Comment 9 Joe Drew (not getting mail) 2012-01-12 15:33:30 PST
*** Bug 704512 has been marked as a duplicate of this bug. ***
Comment 10 Daniel Veditz [:dveditz] 2012-01-18 16:29:50 PST
*** Bug 719202 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.