zlib: handle infback case in NEON optimization |
||||||
Issue descriptionThere are 2 entry points in zlib for decompressing content: - inflate: where an internally managed zlib buffer is used. This is the original exposed API in zlib (released in 1995) - InflateBack: where the user can supply an external buffer. This is a 'newer' API, published around 2002-2003 IIRC. The NEON optimization assumes that it can perform wide stores sometimes overwriting data on the output pointer (but never overflowing the buffer end as it has enough room for the write). For infback there is no such guarantees (i.e. no extra wiggle room), which can result in illegal operations. This is a benign case as the InflateBack interface is not used in Chromium code base but should be handled.
,
Sep 28 2017
Further discussion: https://chromium-review.googlesource.com/c/chromium/src/+/641575/4/third_party/zlib/contrib/arm/chunkcopy.h#230
,
Sep 28 2017
It will basically look like this: https://gist.github.com/Adenilson/0525bb60f79fcc90b4ccedb778a198d1
,
Sep 28 2017
,
Sep 29 2017
Lifting a snippet from the discussion at https://chromium-review.googlesource.com/c/chromium/src/+/641575/4/third_party/zlib/contrib/arm/chunkcopy.h#230 that was linked to by comment #2, some more context: Starting from the zlib-the-library's public API, there are two paths to get to inflate_fast. One is via the inflate function, in inflate.c. Two is via the inflateBack function, in infback.c. The former is used for 'wrapped' wire formats like gzip and zlib-the-format (RFCs 1950 and 1952). The latter is used for 'raw' flate (RFC 1951). Zlib-the-library users like libpng and HTTP stacks usually go through the former. Zip libraries usually go through the latter.
,
Sep 29 2017
> This is a benign case as the InflateBack interface is not used in Chromium code base but should be handled. That sounds plausible, in that https://cs.chromium.org/search/?q=inflateBack&sq=package:chromium&type=cs only lists third_party/zlib and third_party/perl. On the other hand, I believe that chrome extensions use unzip. It might be worth confirming that installing chrome extensions end up going through inflate and not infbackBack.
,
Sep 29 2017
infbackBack should obviously be inflateBack. Its filename is infback.c and I typo'ed the two together.
,
Oct 26 2017
,
Oct 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/626df311481f7fac07b58799fbc94e09c848f01d commit 626df311481f7fac07b58799fbc94e09c848f01d Author: Adenilson Cavalcanti <adenilson.cavalcanti@arm.com> Date: Mon Oct 30 18:39:29 2017 Isolating ARM specific code in inffast The NEON specific code will be hosted in the folder 'contrib/optimizations/arm' while the platform independent C code is hosted in the upper directory. This allows to easily implement the inffast optimization for other architectures by simply implementing 2 functions and including the necessary header in chunk_copy.h (that is used by inflate and inffast). The idea is with time to move all optimizations to this new folder. Bug: 769880 Change-Id: I404ec0fdf3f6867c9c124da859ca38bf57b25447 Reviewed-on: https://chromium-review.googlesource.com/740907 Reviewed-by: Chris Blume <cblume@chromium.org> Commit-Queue: Adenilson Cavalcanti <cavalcantii@chromium.org> Cr-Commit-Position: refs/heads/master@{#512542} [modify] https://crrev.com/626df311481f7fac07b58799fbc94e09c848f01d/third_party/zlib/BUILD.gn [add] https://crrev.com/626df311481f7fac07b58799fbc94e09c848f01d/third_party/zlib/contrib/optimizations/arm/chunkcopy_arm.h [rename] https://crrev.com/626df311481f7fac07b58799fbc94e09c848f01d/third_party/zlib/contrib/optimizations/chunkcopy.h [rename] https://crrev.com/626df311481f7fac07b58799fbc94e09c848f01d/third_party/zlib/contrib/optimizations/inffast_chunky.c [rename] https://crrev.com/626df311481f7fac07b58799fbc94e09c848f01d/third_party/zlib/contrib/optimizations/inflate.c
,
Nov 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0bb11040792edc5b28fcb710fc4c01fedd98c97c commit 0bb11040792edc5b28fcb710fc4c01fedd98c97c Author: Adenilson Cavalcanti <adenilson.cavalcanti@arm.com> Date: Fri Nov 03 17:49:46 2017 Fix InflateBack corner case This handles the case where a zlib user could rely on InflateBack API to decompress content. The NEON optimization assumes that it can perform wide stores, sometimes overwriting data on the output pointer (but never overflowing the buffer end as it has enough room for the write). For infback there is no such guarantees (i.e. no extra wiggle room), which can result in illegal operations. This patch fixes the potential issue by falling back to the non-optimized code for such cases. Also it adds some comments about the entry assumptions in inflate and writes out a defined value at the write buffer to identify where the real data has ended (helpful while debugging). Bug: 769880 Change-Id: I08130e4d012e55fb1a114394b88df7e9d8a6542f Reviewed-on: https://chromium-review.googlesource.com/749732 Reviewed-by: Chris Blume <cblume@chromium.org> Commit-Queue: Adenilson Cavalcanti <cavalcantii@chromium.org> Cr-Commit-Position: refs/heads/master@{#513830} [modify] https://crrev.com/0bb11040792edc5b28fcb710fc4c01fedd98c97c/third_party/zlib/BUILD.gn [modify] https://crrev.com/0bb11040792edc5b28fcb710fc4c01fedd98c97c/third_party/zlib/contrib/optimizations/inffast_chunky.c [add] https://crrev.com/0bb11040792edc5b28fcb710fc4c01fedd98c97c/third_party/zlib/contrib/optimizations/inffast_chunky.h [modify] https://crrev.com/0bb11040792edc5b28fcb710fc4c01fedd98c97c/third_party/zlib/contrib/optimizations/inflate.c
,
Nov 3 2017
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by cavalcantii@chromium.org
, Sep 28 2017