Puffin incorrectly identifies the number of bits to cache in very rare corner cases |
|||||
Issue descriptionb/121032138 found an issue with the puffin client. 1214/024820:ERROR:puffer.cc(173)] br->CacheBits(cur_ht->DistanceMaxBits()) failed. [1214/024820:ERROR:utils.cc(348)] puffer.PuffDeflate(&bit_reader, &puff_writer, nullptr) failed. [1214/024820:ERROR:puffdiff.cc(113)] FindPuffLocations(stream, deflates, puffs, &puff_size) failed. [1214/024820:ERROR:puffdiff.cc(129)] puff_deflate_stream(std::move(dst), dst_deflates, &dst_puff_buffer, &dst_puffs) failed. [1214/024820:ERROR:delta_diff_utils.cc(852)] puffin::PuffDiff(old_data, new_data, src_deflates, dst_deflates, temp_file_path, &puffdiff_delta) failed. [1214/024820:ERROR:delta_diff_utils.cc(634)] ReadExtentsToDiff(old_part, new_part, old_extents_chunk, new_extents_chunk, old_deflates, new_deflates, version, &data, &operation) failed. [1214/024820:ERROR:delta_diff_utils.cc(264)] Failed to generate delta for /opt/google/containers/android/system.raw.img/squashfs-root/system/framework/boot-framework.vdex (2141 blocks) After looking into the issue, it seems that the puffin does not correctly calculates the maximum number of bits it needs to cache to get the output. In the specific instance in b/121032138, the maximum size of encoded distance values were 12 (https://android.git.corp.google.com/platform/external/puffin/+/6d30f049ed4cfc5d4bfa4fdbf6529c9e3522e951/src/puffer.cc#173). But at the end of the stream, there is a distance encoded with just one bit and the end of block symbol encoded with just 10 bits (1 + 10 = 11) and hence we don't need the whole 12 bits for that. We have never encountered this for a long time when puffin has been out there, and case so this is a very rare case.
,
Dec 17
So the issue is that we are always caching the maximum bits needed for distance, but we may not need that many bits, and if we are at the end of the stream, there aren't enough bits left so it failed? Looks like litlen doesn't have this issue: https://android.git.corp.google.com/platform/external/puffin/+/6d30f049ed4cfc5d4bfa4fdbf6529c9e3522e951/src/puffer.cc#135 Neither are good options but 2 sounds better to me.
,
Dec 17
Yeah, litlen doesn't have the issue because eventually we're gonna hit the end of block symbol and we should have enough space in there. But distance bits comes after litlen and get to the problem we have. Can you think of any better options other than these? The other options is to completely disable puffin on older versions, but with that we miss very benefits of it.
,
Dec 17
Wait, isn't the end of block symbol encoded using litlen huffman code?
,
Dec 17
Yes it does. but the problem happens when the number of bits for distance + number of bits for end of block huffman code is smaller than the maximum number of bits needed for distance. It really has nothing to do with the end of block symbol code. So basically we have a distance + end of block that is smaller than the maximum number of bits a distance is needed.
,
Dec 17
The reason it is rare is that this situation should exist and also happen way at the end of the block.
,
Dec 18
,
Jan 2
,
Jan 7
These two changes landed in AOSP for the fix: https://android-review.googlesource.com/c/platform/system/update_engine/+/860831 https://android-review.googlesource.com/c/platform/external/puffin/+/860636 Once we merge them in CrOS, this will be fixed.
,
Jan 8
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/6e51cfbec121fd70c3e79fe1a7c2a18fb7c2e4d0 commit 6e51cfbec121fd70c3e79fe1a7c2a18fb7c2e4d0 Author: Amin Hassani <ahassani@chromium.org> Date: Tue Jan 08 03:40:59 2019 Marking 9999 ebuild for dev-util/puffin as stable. Pulling in https://android-review.googlesource.com/c/platform/external/puffin/+/860636 BUG= chromium:915559 TEST=unittests Change-Id: I4005df927c7223101def25d36e9fd146a37e6768 Reviewed-on: https://chromium-review.googlesource.com/1399261 Commit-Ready: Amin Hassani <ahassani@chromium.org> Tested-by: Amin Hassani <ahassani@chromium.org> Reviewed-by: Mike Frysinger <vapier@chromium.org> [rename] https://crrev.com/6e51cfbec121fd70c3e79fe1a7c2a18fb7c2e4d0/dev-util/puffin/puffin-1.0.0-r426.ebuild
,
Jan 8
,
Jan 8
The way we implemented the fix does requires an update engine merge to M72 which is not feasible at this point. I'm suggesting we target the fix for M73 and if the issue happend in M72, we do the same thing we did for M71 and block the puffdiff directly.
,
Jan 11
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by ahass...@chromium.org
, Dec 17