New issue
Advanced search Search tips

Issue 877236 link

Starred by 1 user
Status: Verified
Owner:
manojgupta@chromium.org
Closed: Aug 27
Cc:
vapier@chromium.org
metzman@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug
Build-Toolchain



Sign in to add a comment

Undefined behavior in bzip2

Project Member Reported by manojgupta@chromium.org, Aug 23 
caaught this when running puffin with full system instrumented ubsan.

 * ASAN error detected:
 * ../bzip2-1.0.6/blocksort.c:255:7: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
 *     #0 0x7fa28d834218 in fallbackSort /build/amd64-generic/tmp/portage/app-arch/bzip2-1.0.6-r8/work/bzip2-1.0.6-abi_x86_64.amd64/../bzip2-1.0.6/blocksort.c:0:21
 *     #1 0x7fa28d831ab5 in BZ2_blockSort /build/amd64-generic/tmp/portage/app-arch/bzip2-1.0.6-r8/work/bzip2-1.0.6-abi_x86_64.amd64/../bzip2-1.0.6/blocksort.c:0:28
 *     #2 0x7fa28d83721b in BZ2_compressBlock /build/amd64-generic/tmp/portage/app-arch/bzip2-1.0.6-r8/work/bzip2-1.0.6-abi_x86_64.amd64/../bzip2-1.0.6/compress.c:616:7
 *     #3 0x7fa28d84f84e in handle_compress /build/amd64-generic/tmp/portage/app-arch/bzip2-1.0.6-r8/work/bzip2-1.0.6-abi_x86_64.amd64/../bzip2-1.0.6/bzlib.c:0:51
 *     #4 0x7fa28d84e693 in BZ2_bzCompress /build/amd64-generic/tmp/portage/app-arch/bzip2-1.0.6-r8/work/bzip2-1.0.6-abi_x86_64.amd64/../bzip2-1.0.6/bzlib.c:456:21
 *     #5 0x7fa28ecd2929 in bsdiff::BZ2Compressor::Finish() /build/amd64-generic/var/cache/portage/dev-util/bsdiff/out/Default/../../../../../../../tmp/portage/dev-util/bsdiff-4.3.1-r16/work/bsdiff-4.3.1/platform2/bsdiff/bz2_compressor.cc:77:14
 *     #6 0x7fa28ecdb6d9 in bsdiff::BsdiffPatchWriter::SelectSmallestResult(std::__1::vector<std::__1::unique_ptr<bsdiff::CompressorInterface, std::__1::default_delete<bsdiff::CompressorInterface> >, std::__1::allocator<std::__1::unique_ptr<bsdiff::CompressorInterface, std::__1::default_delete<bsdiff::CompressorInterface> > > > const&, bsdiff::CompressorInterface**) /build/amd64-generic/var/cache/portage/dev-util/bsdiff/out/Default/../../../../../../../tmp/portage/dev-util/bsdiff-4.3.1-r16/work/bsdiff-4.3.1/platform2/bsdiff/patch_writer.cc:142:22
 *     #7 0x7fa28ecdb9d9 in bsdiff::BsdiffPatchWriter::Close() /build/amd64-generic/var/cache/portage/dev-util/bsdiff/out/Default/../../../../../../../tmp/portage/dev-util/bsdiff-4.3.1-r16/work/bsdiff-4.3.1/platform2/bsdiff/patch_writer.cc:165:8
 *     #8 0x7fa28ecd5204 in bsdiff::DiffEncoder::Close() /build/amd64-generic/var/cache/portage/dev-util/bsdiff/out/Default/../../../../../../../tmp/portage/dev-util/bsdiff-4.3.1-r16/work/bsdiff-4.3.1/platform2/bsdiff/diff_encoder.cc:85:18
 *     #9 0x7fa28ecd1a23 in bsdiff::bsdiff(unsigned char const*, unsigned long, unsigned char const*, unsigned long, unsigned long, bsdiff::PatchWriterInterface*, bsdiff::SuffixArrayIndexInterface**) /build/amd64-generic/var/cache/portage/dev-util/bsdiff/out/Default/../../../../../../../tmp/portage/dev-util/bsdiff-4.3.1-r16/work/bsdiff-4.3.1/platform2/bsdiff/bsdiff.cc:183:20
 *     #10 0x7fa28ecd1b59 in bsdiff::bsdiff(unsigned char const*, unsigned long, unsigned char const*, unsigned long, bsdiff::PatchWriterInterface*, bsdiff::SuffixArrayIndexInterface**) /build/amd64-generic/var/cache/portage/dev-util/bsdiff/out/Default/../../../../../../../tmp/portage/dev-util/bsdiff-4.3.1-r16/work/bsdiff-4.3.1/platform2/bsdiff/bsdiff.cc:60:9
 *     #11 0x55af84a7bd6b in puffin::PuffDiff(std::__1::unique_ptr<puffin::StreamInterface, std::__1::default_delete<puffin::StreamInterface> >, std::__1::unique_ptr<puffin::StreamInterface, std::__1::default_delete<puffin::StreamInterface> >, std::__1::vector<puffin::BitExtent, std::__1::allocator<puffin::BitExtent> > const&, std::__1::vector<puffin::BitExtent, std::__1::allocator<puffin::BitExtent> > const&, std::__1::vector<bsdiff::CompressorType, std::__1::allocator<bsdiff::CompressorType> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >*) /build/amd64-generic/var/cache/portage/dev-util/puffin/out/Default/../../../../../../../tmp/portage/dev-util/puffin-1.0.0-r423/work/puffin-1.0.0/platform2/puffin/src/puffdiff.cc:134:3
 *     #12 0x55af84a7d697 in puffin::PuffDiff(std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > const&, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > const&, std::__1::vector<puffin::BitExtent, std::__1::allocator<puffin::BitExtent> > const&, std::__1::vector<puffin::BitExtent, std::__1::allocator<puffin::BitExtent> > const&, std::__1::vector<bsdiff::CompressorType, std::__1::allocator<bsdiff::CompressorType> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >*) /build/amd64-generic/var/cache/portage/dev-util/puffin/out/Default/../../../../../../../tmp/portage/dev-util/puffin-1.0.0-r423/work/puffin-1.0.0/platform2/puffin/src/puffdiff.cc:161:10
 *     #13 0x55af84a4a681 in puffin::TestPatching(std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > const&, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > const&, std::__1::vector<puffin::BitExtent, std::__1::allocator<puffin::BitExtent> > const&, std::__1::vector<puffin::BitExtent, std::__1::allocator<puffin::BitExtent> > const&, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> >) /build/amd64-generic/var/cache/portage/dev-util/puffin/out/Default/../../../../../../../tmp/portage/dev-util/puffin-1.0.0-r423/work/puffin-1.0.0/platform2/puffin/src/patching_unittest.cc:140:3
 *     #14 0x55af84a4b2fe in puffin::PatchingTest_Patching1To2Test_Test::TestBody() /build/amd64-generic/var/cache/portage/dev-util/puffin/out/Default/../../../../../../../tmp/portage/dev-util/puffin-1.0.0-r423/work/puffin-1.0.0/platform2/puffin/src/patching_unittest.cc:158:3
 *     #15 0x7fa28edeb299 in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /build/amd64-generic/tmp/portage/dev-cpp/gtest-1.8.0-r1/work/googletest-release-1.8.0/googletest-abi_x86_64.amd64/./src/gtest.cc:2402:10
 *     #16 0x7fa28edeb299 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /build/amd64-generic/tmp/portage/dev-cpp/gtest-1.8.0-r1/work/googletest-release-1.8.0/googletest-abi_x86_64.amd64/./src/gtest.cc:2438:0
 *     #17 0x7fa28edb1a78 in testing::Test::Run() /build/amd64-generic/tmp/portage/dev-cpp/gtest-1.8.0-r1/work/googletest-release-1.8.0/googletest-abi_x86_64.amd64/./src/gtest.cc:2474:5
 *     #18 0x7fa28edb42da in testing::TestInfo::Run() /build/amd64-generic/tmp/portage/dev-cpp/gtest-1.8.0-r1/work/googletest-release-1.8.0/googletest-abi_x86_64.amd64/./src/gtest.cc:2656:11
 *     #19 0x7fa28edb581c in testing::TestCase::Run() /build/amd64-generic/tmp/portage/dev-cpp/gtest-1.8.0-r1/work/googletest-release-1.8.0/googletest-abi_x86_64.amd64/./src/gtest.cc:2774:28
 *     #20 0x7fa28edc6a4d in testing::internal::UnitTestImpl::RunAllTests() /build/amd64-generic/tmp/portage/dev-cpp/gtest-1.8.0-r1/work/googletest-release-1.8.0/googletest-abi_x86_64.amd64/./src/gtest.cc:4649:43
 *     #21 0x7fa28ededa29 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /build/amd64-generic/tmp/portage/dev-cpp/gtest-1.8.0-r1/work/googletest-release-1.8.0/googletest-abi_x86_64.amd64/./src/gtest.cc:2402:10
 *     #22 0x7fa28ededa29 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /build/amd64-generic/tmp/portage/dev-cpp/gtest-1.8.0-r1/work/googletest-release-1.8.0/googletest-abi_x86_64.amd64/./src/gtest.cc:2438:0
 *     #23 0x7fa28edc62a7 in testing::UnitTest::Run() /build/amd64-generic/tmp/portage/dev-cpp/gtest-1.8.0-r1/work/googletest-release-1.8.0/googletest-abi_x86_64.amd64/./src/gtest.cc:4257:10
 *     #24 0x55af84a85511 in RUN_ALL_TESTS() /build/amd64-generic/var/cache/portage/dev-util/puffin/out/Default/../../../../../../../usr/include/gtest/gtest.h:2233:46
 *     #25 0x55af84a85511 in main /build/amd64-generic/var/cache/portage/dev-util/puffin/out/Default/../../../../../../../tmp/portage/dev-util/puffin-1.0.0-r423/work/puffin-1.0.0/platform2/common-mk/testrunner.cc:16:0
 *     #26 0x7fa28d893735 in __libc_start_main /var/tmp/portage/cross-x86_64-cros-linux-gnu/glibc-2.23-r18/work/glibc-2.23/csu/../csu/libc-start.c:289:0
 *     #27 0x55af84a21648 in _start ??:0:0

Comment 1 by metzman@chromium.org, Aug 23 

Does this bug appear spurious to you?

Comment 2 by manojgupta@chromium.org, Aug 23

Cc: vapier@chromium.org
Summary: Undefined behavior in bzip2 (was: Undefined behavior in bzip)
This is probably harmless but easy to fix as well. 

Only issue is I can't find where to send the fix for this since  https://sourceware.org/bzip2/ doesn't have a link to a repo. 

Vapier@ Any ideas where should I send the bzip2 patch to?

Comment 3 by vapier@chromium.org, Aug 23 

just send the patch to jseward@acm.org, but don't hold your breath it'll get merged

for Gentoo, send a PR via GH to get it merged there

Comment 4 by manojgupta@chromium.org, Aug 24 

Sent the email.

Gentoo pull request is at https://github.com/gentoo/gentoo/pull/9688
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 25 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/portage-stable/+/662484ae8b26971216f817caf3932869b564a24c

commit 662484ae8b26971216f817caf3932869b564a24c
Author: Manoj Gupta <manojgupta@google.com>
Date: Sat Aug 25 14:52:17 2018

bzip2: Fix an issue with shift caught by ubsan.

Apply the patch already sent/applied in Gentoo.

https://github.com/gentoo/gentoo/commit/66f614c51f017b0693f5aaeb5897db28ef3aff6c

Patch details:
Use unsigned 1 for shifting instead of signed 1.

This fixed an issue with shift caught by undefined behavior
sanitizer in clang.
bzip2-1.0.6/blocksort.c:255:7
runtime error: left shift of 1 by 31 places cannot be represented in type 'int'

BUG= chromium:877236 
TEST=bzip2 builds.

Change-Id: If9de5b5d5b1dd2f676b8921fe19cec4a6e8c4430
Reviewed-on: https://chromium-review.googlesource.com/1188822
Tested-by: Manoj Gupta <manojgupta@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Commit-Queue: Manoj Gupta <manojgupta@chromium.org>

[rename] https://crrev.com/662484ae8b26971216f817caf3932869b564a24c/app-arch/bzip2/bzip2-1.0.6-r9.ebuild
[add] https://crrev.com/662484ae8b26971216f817caf3932869b564a24c/app-arch/bzip2/files/bzip2-1.0.6-ubsan-error.patch

Comment 6 by manojgupta@chromium.org, Aug 27

Status: Verified (was: Untriaged)
►

Sign in to add a comment