726586 - Uninitialised value use in build_input_gamma_table

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[Julian Seward [:jseward]]

Running on a 16 bit deep VNC server, if that has any relevance.

TEST_PATH=dom/tests/mochitest/dom-level2-html/test_HTMLFrameElement01.html

(DISPLAY=:3.0 make -C ff-opt mochitest-plain TEST_PATH=dom/tests/mochitest/dom-level2-html/test_HTMLFrameElement01.html EXTRA_TEST_ARGS='--close-when-done --debugger=/home/sewardj/VgTRUNK/merge/Inst/bin/valgrind --debugger-args="--smc-check=all-non-file --suppressions=/home/sewardj/MOZ/SUPPS/mochitest-mc.supp --error-limit=no --stats=yes --trace-children=yes --child-silent-after-fork=yes '--trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel,*/bin/uname,*/bin/which,*/bin/ps,*/bin/grep' --tool=memcheck --track-origins=yes --stats=yes"') 2>&1 | cat

Conditional jump or move depends on uninitialised value(s)
   at 0x702BE57: build_input_gamma_table (transform_util.c:224)
   by 0x702B63B: qcms_transform_create (transform.c:1194)
   by 0x61E92F2: mozilla::image::nsPNGDecoder::info_callback(png_struct_def*, png_info_struct*) (nsPNGDecoder.cpp:578)
   by 0x6E5F337: MOZ_PNG_push_read_chunk (pngpread.c:1900)
   by 0x6E5F79A: MOZ_PNG_process_data (pngpread.c:41)
   by 0x61E8CA2: mozilla::image::nsPNGDecoder::WriteInternal(char const*, unsigned int) (nsPNGDecoder.cpp:357)
   by 0x61D0C61: mozilla::image::RasterImage::WriteToDecoder(char const*, unsigned int) (RasterImage.cpp:2373)
   by 0x61D1E79: mozilla::image::imgDecodeWorker::Run() (RasterImage.cpp:2823)
   by 0x61D2554: mozilla::image::RasterImage::AddSourceData(char const*, unsigned int) (RasterImage.cpp:1499)
   by 0x61D2719: mozilla::image::RasterImage::WriteToRasterImage(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) (RasterImage.cpp:2921)
   by 0x6D791F2: nsInputStreamTee::WriteSegmentFun(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) (nsInputStreamTee.cpp:223)
   by 0x6D7E71E: nsPipeInputStream::ReadSegments(unsigned int (*)(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) (nsPipe3.cpp:799)

 Uninitialised value was created by a heap allocation
   at 0x4029B9A: malloc (vg_replace_malloc.c:263)
   by 0x7026B22: curve_from_gamma (iccread.c:846)
   by 0x7028D4D: qcms_profile_create_rgb_with_gamma (iccread.c:875)
   by 0x61E9AAB: mozilla::image::nsPNGDecoder::info_callback(png_struct_def*, png_info_struct*) (nsPNGDecoder.cpp:469)
   by 0x6E5F337: MOZ_PNG_push_read_chunk (pngpread.c:1900)
   by 0x6E5F79A: MOZ_PNG_process_data (pngpread.c:41)
   by 0x61E8CA2: mozilla::image::nsPNGDecoder::WriteInternal(char const*, unsigned int) (nsPNGDecoder.cpp:357)
   by 0x61D0C61: mozilla::image::RasterImage::WriteToDecoder(char const*, unsigned int) (RasterImage.cpp:2373)
   by 0x61D1E79: mozilla::image::imgDecodeWorker::Run() (RasterImage.cpp:2823)
   by 0x61D2554: mozilla::image::RasterImage::AddSourceData(char const*, unsigned int) (RasterImage.cpp:1499)
   by 0x61D2719: mozilla::image::RasterImage::WriteToRasterImage(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) (RasterImage.cpp:2921)
   by 0x6D791F2: nsInputStreamTee::WriteSegmentFun(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) (nsInputStreamTee.cpp:223)

Comment 1

[Julian Seward [:jseward]]

Possibly is a false positive induced by insufficiently precise
Memcheck instrumentation of MOVMSKB.

Comment 2

[Julian Seward [:jseward]]

This is one of the last remaining errors reported by recent
mochitest/valgrind runs.

I analysed it further and now believe it is a real error.
AFAICS the code allocates an object of type 'struct curveType' on
the heap, and subsequently does an if.. based on its .type field,
without setting that field to any value.

The neatened up Valgrind report is as follows (read the second stack
first):

Conditional jump or move depends on uninitialised value(s)
   at 0x7119AA5: build_input_gamma_table (gfx/qcms/transform_util.c:199)
   by 0x711929B: qcms_transform_create (gfx/qcms/transform.c:1211)
   by 0x62DE1C0: mozilla::image::nsPNGDecoder::info_callback(...)
                 (image/decoders/nsPNGDecoder.cpp:569)
   by 0x72CE84B: MOZ_PNG_push_read_chunk (media/libpng/pngpread.c:1446)

 Uninitialised value was created by a heap allocation
   at 0x402ADDC: malloc (vg_replace_malloc.c:270)
   by 0x7113ED2: curve_from_gamma (gfx/qcms/iccread.c:846)
   by 0x711606D: qcms_profile_create_rgb_with_gamma (gfx/qcms/iccread.c:875)
   by 0x62DDA08: mozilla::image::PNGGetColorProfile(...)
                 (image/decoders/nsPNGDecoder.cpp:461)
   by 0x62DE4D0: mozilla::image::nsPNGDecoder::info_callback(...)
                 (image/decoders/nsPNGDecoder.cpp:552)
   by 0x72CE84B: MOZ_PNG_push_read_chunk (media/libpng/pngpread.c:1446)



Here is the allocating call chain for the object:

---> nsPNGDecoder.cpp
551    decoder->mInProfile = PNGGetColorProfile(png_ptr, info_ptr,
552                                             color_type, &inType, &pIntent);
---> nsPNGDecoder.cpp
460    profile = qcms_profile_create_rgb_with_gamma(whitePoint, primaries,
461                                                 1.0/gammaOfFile);
---> iccread.c
875	profile->redTRC = curve_from_gamma(gamma);
---> iccread.c
846	curve = malloc(sizeof(struct curveType) + sizeof(uInt16Number)*num_entries);
847	if (!curve)
848		return NULL;
849	curve->count = num_entries;
850	curve->data[0] = float_to_u8Fixed8Number(gamma);
851	return curve;
// 	struct curveType *curve;

Note it doesn't set curve->type.


and here's the chain leading to the use of that field:

---> nsPNGDecoder.cpp
565    decoder->mTransform = qcms_transform_create(decoder->mInProfile,
566                                           inType,
567                                           gfxPlatform::GetCMSOutputProfile(),
568                                           outType,
569                                           (qcms_intent)intent);
---> transform.c
1211		transform->input_gamma_table_r = build_input_gamma_table(in->redTRC);
---> transform_util.c
199		if (TRC->type == PARAMETRIC_CURVE_TYPE) {
// struct curveType *TRC


Note, both chains are 'rooted' in nsPNGDecoder.cpp,
nsPNGDecoder::info_callback(), lines 551 (allocation chain) and 565
(usage chain).

Comment 3

[Joe Drew (not getting mail)]

For once, a QCMS error actually seems to be a bug in QCMS and not imagelib.

Comment 4

[Julian Seward [:jseward]]

Attachment: trivial fix

This stops valgrind complaining.  I have no idea if it is correct.
AFAICS zero isn't a valid value for struct curveType::type, judging
by the list of *_TYPE #defines starting at iccread.c:347.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: patch

Thanks Julian.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

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

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

::: gfx/qcms/iccread.c
@@ +847,5 @@
>  	if (!curve)
>  		return NULL;
>  	curve->count = num_entries;
>  	curve->data[0] = float_to_u8Fixed8Number(gamma);
> +  curve->type = CURVE_TYPE;

make sure the indentation is right

Comment 7

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86e10b84313b

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/86e10b84313b