search

cxt / webp

Automatically exported from code.google.com/p/webp
0 stars 0 forks source link

scaled decodes seem to use uninitialized memory #254

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago 
In Skia (skia.org), we are using libwebp version 0.4.3.

We run tests that decode an image (at various scales) and then draw the result. 
Then we compare the results over time to see if we've made a change that 
generated a different result.

When we decode a webp image scaled, we see results that are different on almost 
every run. Most of the image looks the same, but there are some pixels along 
the right edge (that seem to be somewhat evenly spaced) that look slightly 
different.

We also run a valgrind bot which complains of using uninitialized memory, which 
I suspect is related. (An example is here: 
https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x
86_64-Release-Valgrind/builds/295/steps/dm/logs/stdio.)

I have attached a sample image along with a couple of results (scaled to 0.625) 
and the difference between these two results.

Original issue reported on code.google.com by scro...@google.com on 13 Jul 2015 at 2:56

Attachments:

GoogleCodeExporter commented 8 years ago 
Thanks for the report. I imagine we'll be able to reproduce the report with 
cwebp. Just for definiteness can you give the dimensions you were scaling to 
rather than just the factor?

Original comment by jz...@google.com on 14 Jul 2015 at 4:15

GoogleCodeExporter commented 8 years ago 
This particular image was scaled to 200 x 147 in this instance, although we 
found similar behavior at several scales.

Original comment by scro...@google.com on 14 Jul 2015 at 4:43

GoogleCodeExporter commented 8 years ago 
I can reproduce this doing an upscale with 0.4.3 and the current tip of tree. A 
good way to achieve a difference is with different values of --malloc-fill:

$ valgrind \
  --track-origins=yes \
  --malloc-fill=0xcc \
  ./examples/dwebp \
  -ppm -scale 200 147 \
  5.sm.webp -o out.ppm

I'll have a closer look when I get a chance.

Original comment by jz...@google.com on 18 Jul 2015 at 2:54

GoogleCodeExporter commented 8 years ago 
there's apparently a buffer-read overflow happening at src/utils/rescaler.c:51, 
where 'x_in' can become equal to 'wrk->src_width' (shouldn't happen). 
Investigating and fixing...

Original comment by pascal.m...@gmail.com on 23 Jul 2015 at 5:01

GoogleCodeExporter commented 8 years ago 
Right one could clamp or perhaps bias 'accum' given the current precision. 
Increasing the precision would likely give a more accurate calculation of this 
coordinate.

Original comment by jz...@google.com on 23 Jul 2015 at 9:27

GoogleCodeExporter commented 8 years ago 
patch https://chromium-review.googlesource.com/#/c/290560/ should address the 
issue and is out for review.

Original comment by pascal.m...@gmail.com on 5 Aug 2015 at 1:36

GoogleCodeExporter commented 8 years ago 
the patches have been submitted.
It should address the issue. Can you cross-check please?

Thanks for the report!

Original comment by s...@google.com on 7 Aug 2015 at 12:10

GoogleCodeExporter commented 8 years ago 
Thanks for the fix!

I'll make sure it works in Skia. The Skia bug is tracked in 
https://code.google.com/p/skia/issues/detail?id=4038 - you can follow along 
there, but I'll report back here if it still shows problems.

Original comment by scro...@google.com on 7 Aug 2015 at 7:43

GoogleCodeExporter commented 8 years ago 
From the comment in https://codereview.chromium.org/1280073002/#msg13 it sounds 
like this is still not fixed for odd heights?

Original comment by scro...@google.com on 7 Aug 2015 at 8:07

GoogleCodeExporter commented 8 years ago 
Correct, we'll get a fix in for that soon. The problem there is not an out of 
bounds read, but an incorrect final row.

Original comment by jz...@google.com on 8 Aug 2015 at 5:02

GoogleCodeExporter commented 8 years ago 
I just realized this is still marked as fixed. Did you fix the problem with odd 
heights? Or is it tracked in another bug?

Original comment by scro...@google.com on 28 Aug 2015 at 8:26

GoogleCodeExporter commented 8 years ago 
Reopening to track the remaining final row issue, fix is still pending.

Original comment by jz...@google.com on 29 Aug 2015 at 1:09

GoogleCodeExporter commented 8 years ago 
The fix has been merged, all issues in this bug should be resolved.

https://chromium-review.googlesource.com/#/c/299840/
5ff0079 fix rescaler vertical interpolation

Original comment by jz...@google.com on 19 Sep 2015 at 1:18

GoogleCodeExporter commented 8 years ago 
 follow-up note: some notable speed improvement to the rescaler code submitted recently:

NEON: 
https://chromium.googlesource.com/webm/libwebp/+/b4e731cd93f9e86e9ea8cde7a0c0aa5
4ca0d777d
SSE2: 
https://chromium.googlesource.com/webm/libwebp/+/76a7dc39e57ae192f521779c81c6e0b
8c5bd4e85https://chromium.googlesource.com/webm%2Flibwebp/+/932fd4df61c90b52e69c
391b13b32ba22f8149f3

Original comment by pascal.m...@gmail.com on 10 Oct 2015 at 6:05