Innocuous uninitialized value in nsBMPDecoder

RESOLVED FIXED in mozilla11

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: bbondy)

Tracking

Trunk
mozilla11
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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==
I thought this was known and innocuous?
(Assignee)

Comment 2

6 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 700930
(Assignee)

Comment 3

6 years ago
It turns out that this bug is not related to what I thought was the cause of bug 700930.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Comment 4

6 years ago
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.
Attachment #574405 - Flags: review?(joe)
(Assignee)

Updated

6 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Comment 5

6 years ago
"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.
(Reporter)

Updated

6 years ago
Attachment #574405 - Flags: review?(joe) → review+
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Updated

6 years ago
Summary: Uninitialized values in nsBMPDecoder → Innocuous uninitialized value in nsBMPDecoder
(Assignee)

Updated

6 years ago
Target Milestone: --- → mozilla10
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
(Assignee)

Comment 7

6 years ago
This was pushed to m-i earlier today by the way:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d339796e649b
https://hg.mozilla.org/mozilla-central/rev/d339796e649b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla10 → mozilla11
(Reporter)

Updated

5 years ago
Duplicate of this bug: 704512
Duplicate of this bug: 719202
You need to log in before you can comment on or make changes to this bug.