1262905 - JPEG triggers multiple undefined memory usage reports [@ClampTo8]

Status: RESOLVED WONTFIX

(Core :: Graphics: ImageLib, defect, P3)

Description

[Tyson Smith [:tsmith]]

Attachment: valgrind_log.txt

Multiple undefined memory usage reports from valgrind.

Please see attached valgrind log.

Load test_case.html to reproduce these issues.

Some reports are also present in bug 1262903 but not all.

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 2

[Tyson Smith [:tsmith]]

Attachment: test_case.jpg

Comment 3

[DRC]

Valgrind log indicates that the issue is not in libjpeg-turbo code, and the issue is not reproducible in standalone libjpeg-turbo.  Removing myself.  Please do not add me in the future unless the issue can be shown to affect libjpeg-turbo.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: libjpeg_turbo_test.tar.gz

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8809436 [details]
libjpeg_turbo_test.tar.gz

I created a simple test case based off the example code and linked against the latest libjpeg-turbo off github. The issue reproduces and appears to be caused by the SSE2 optimized algorithm in jdcolext-sse2-64.asm. If I force it to use the C version, the valgrind error goes away. I believe this is probably a false positive as the output is consistently the same both in my test case and in libjpeg, but if you could confirm that would be appreciated.

Comment 7

[DRC]

What exactly is the text of the warning/error that valgrind triggers?  Is it the same as this one?

https://github.com/libjpeg-turbo/libjpeg-turbo/issues/15

If so, then a member of our community is already on the case.  I believe that this issue has existed since the very beginning of libjpeg-turbo and that it's due to valgrind not properly detecting that the SSE2 instructions have modified the buffer, but I've been wrong before.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

(In reply to DRC from comment #7)
> What exactly is the text of the warning/error that valgrind triggers?  Is it
> the same as this one?
> 

From the logs in attachment #8809436 [details] :

(123, 0) => FF808080
==26380== Conditional jump or move depends on uninitialised value(s)
==26380==    at 0x5105718: _itoa_word (_itoa.c:180)
==26380==    by 0x510912C: vfprintf (vfprintf.c:1631)
==26380==    by 0x510AEF0: buffered_vfprintf (vfprintf.c:2320)
==26380==    by 0x510832C: vfprintf (vfprintf.c:1293)
==26380==    by 0x51107F6: fprintf (fprintf.c:32)
==26380==    by 0x4012E6: put_scanline_someplace (bug1292905.c:29)
==26380==    by 0x4011C2: read_JPEG_file (example.c:379)
==26380==    by 0x401258: main (bug1292905.c:17)
==26380==  Uninitialised value was created by a heap allocation
==26380==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==26380==    by 0x4E7FC61: alloc_sarray (in /opt/libjpeg-turbo/lib64/libjpeg.so.62.2.0)
==26380==    by 0x4E73858: jinit_upsampler (in /opt/libjpeg-turbo/lib64/libjpeg.so.62.2.0)
==26380==    by 0x4E6DE9C: jinit_master_decompress (in /opt/libjpeg-turbo/lib64/libjpeg.so.62.2.0)
==26380==    by 0x4E5C8A4: jpeg_start_decompress (in /opt/libjpeg-turbo/lib64/libjpeg.so.62.2.0)
==26380==    by 0x401143: read_JPEG_file (example.c:349)
==26380==    by 0x401258: main (bug1292905.c:17)
...
(124, 0) => FF808080

It complains several times as it tries to print the pixel value for (124,0) which is the last pixel in the first row. The pixel value is consistently the same/sane.

> https://github.com/libjpeg-turbo/libjpeg-turbo/issues/15
> 
> If so, then a member of our community is already on the case.  I believe
> that this issue has existed since the very beginning of libjpeg-turbo and
> that it's due to valgrind not properly detecting that the SSE2 instructions
> have modified the buffer, but I've been wrong before.

Thanks for the link, I should have scanned the closed issues. In addition to what is in the bug, I can confirm that it is the jsimd_ycc_extbgrx_convert_sse2 x86-64 implementation for this particular image.

Maybe we can get this fixed in valgrind.

Comment 9

[Tyson Smith [:tsmith]]

(In reply to Andrew Osmond [:aosmond] from comment #8)
> Maybe we can get this fixed in valgrind.

I just tested with Valgrind SVN revision 16470 and the issues is still present.

Julian is this of any interest? Should a Valgrind issue be opened for this?

Comment 10

[Julian Seward [:jseward]]

(In reply to Tyson Smith [:tsmith] from comment #9)
I've been aware of this one for a while, but never had a standalone test case
till now, and so never chased it.  I can chase it if you like.  I assume it
is some inaccuracy in Valgrind's tracking of definedness in SIMD operations.
It may be trivial to fix or it might be a whole big deal.  Can't tell without
looking.

Having chased these kinds of things in the past, though, I would say that it
is likely to indicate that the libjpeg-turbo code in question uses uninitialised
memory in its computations, in a way which it knows is safe.  For example,
imagine we do SIMD multiplication of a vector some of whose lanes are undefined,
with a second vector that contains zero for each undefined lane in the first
vector.  Then the result is completely defined, but Valgrind doesn't "know"
that because it doesn't "know" that zero * undefined = defined(zero).

The above is just an example of what I've seen in the past.  I'm not saying
this is what is actually happening here.

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Continued discussion here:

https://github.com/libjpeg-turbo/libjpeg-turbo/issues/277

I think this is a WONTFIX.

Comment 12

[DRC]

Yes, unless this can be shown to be a legitimate bug in libjpeg-turbo (which is highly doubtful, given the level of scrutiny that the library has undergone in the past eight years and the fact that some form of this issue has existed for all of that time), I am not interested in spending any time on it-- particularly given that it's easy to work around the issue (for testing purposes) by setting the JSIMD_FORCENONE environment variable to 1.  Julian is probably correct that this is due to libjpeg-turbo's hand-rolled NASM code doing something that it knows is OK but valgrind doesn't know is OK.  ARM is working on a wholesale rewrite of our ARM and x86 SIMD extensions using compiler intrinsics, so there's a possibility that this issue will go away once we transition away from NASM code.