Sent a patch trying to fix this. http://reviews.llvm.org/rL273319
On Tue, Jun 21, 2016 at 10:52 PM Eric Liu <ioe...@google.com> wrote: > Thanks Mike! I am working on a fix. > > On Tue, Jun 21, 2016 at 10:44 PM Mike Aizatsky <aizat...@google.com> > wrote: > >> I think this change has buffer overflow. It breaks asan build: >> >> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/13946 >> >> FAIL: Clang-Unit :: >> Tooling/ToolingTests/Range.RangesWithNonOverlappingReplacements (9361 of >> 9474) >> ******************** TEST 'Clang-Unit :: >> Tooling/ToolingTests/Range.RangesWithNonOverlappingReplacements' FAILED >> ******************** >> Note: Google Test filter = Range.RangesWithNonOverlappingReplacements >> [==========] Running 1 test from 1 test case. >> [----------] Global test environment set-up. >> [----------] 1 test from Range >> [ RUN ] Range.RangesWithNonOverlappingReplacements >> ================================================================= >> ==32475==ERROR: AddressSanitizer: global-buffer-overflow on address >> 0x0000078a4b02 at pc 0x000000540095 bp 0x7ffcd9799a10 sp 0x7ffcd97991c0 >> READ of size 5 at 0x0000078a4b02 thread T0 >> #0 0x540094 in __asan_memcpy >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:393 >> #1 0x7f6e902e6dc1 in copy >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/projects/libcxx/include/string:655:50 >> #2 0x7f6e902e6dc1 in std::__1::basic_string<char, >> std::__1::char_traits<char>, std::__1::allocator<char> >::__init(char >> const*, unsigned long) >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/projects/libcxx/include/string:2054 >> #3 0x30b45e3 in basic_string >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/libcxx_build_asan/include/c++/v1/string:2086:5 >> #4 0x30b45e3 in >> clang::tooling::calculateRangesAfterReplacements(std::__1::set<clang::tooling::Replacement, >> std::__1::less<clang::tooling::Replacement>, >> std::__1::allocator<clang::tooling::Replacement> > const&, >> std::__1::vector<clang::tooling::Range, >> std::__1::allocator<clang::tooling::Range> > const&) >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/Tooling/Core/Replacement.cpp:325 >> #5 0xaa1a6c in >> clang::tooling::Range_RangesWithNonOverlappingReplacements_Test::TestBody() >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/unittests/Tooling/RefactoringTest.cpp:503:3 >> #6 0x241400a in HandleExceptionsInMethodIfSupported<testing::Test, void> >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/src/gtest.cc:2145:12 >> #7 0x241400a in testing::Test::Run() >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/src/gtest.cc:2161 >> #8 0x2415426 in testing::TestInfo::Run() >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/src/gtest.cc:2309:5 >> #9 0x241636a in testing::TestCase::Run() >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/src/gtest.cc:2416:5 >> #10 0x242c6fc in testing::internal::UnitTestImpl::RunAllTests() >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/src/gtest.cc:4207:11 >> #11 0x242b808 in >> HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/src/gtest.cc:2145:12 >> #12 0x242b808 in testing::UnitTest::Run() >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/googletest/src/gtest.cc:3841 >> #13 0x24000dd in main >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/utils/unittest/UnitTestMain/TestMain.cpp:48:10 >> #14 0x7f6e8fcdcf44 in __libc_start_main >> (/lib/x86_64-linux-gnu/libc.so.6+0x21f44) >> #15 0x4aeb02 in _start >> (/mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm_build_asan/tools/clang/unittests/Tooling/ToolingTests+0x4aeb02) >> >> 0x0000078a4b02 is located 62 bytes to the left of global variable '<string >> literal>' defined in >> '/mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/include/llvm/ADT/SmallVector.h:150:5' >> (0x78a4b40) of size 13 >> '<string literal>' is ascii string 'idx < size()' >> 0x0000078a4b02 is located 0 bytes to the right of global variable '<string >> literal>' defined in >> '/mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/Tooling/Core/Replacement.cpp:325:49' >> (0x78a4b00) of size 2 >> '<string literal>' is ascii string ' ' >> SUMMARY: AddressSanitizer: global-buffer-overflow >> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:393 >> in __asan_memcpy >> Shadow bytes around the buggy address: >> 0x000080f0c910: 00 00 00 00 00 00 00 00 00 07 f9 f9 f9 f9 f9 f9 >> 0x000080f0c920: 00 00 00 00 00 00 00 00 01 f9 f9 f9 f9 f9 f9 f9 >> 0x000080f0c930: 03 f9 f9 f9 f9 f9 f9 f9 03 f9 f9 f9 f9 f9 f9 f9 >> 0x000080f0c940: 03 f9 f9 f9 f9 f9 f9 f9 02 f9 f9 f9 f9 f9 f9 f9 >> 0x000080f0c950: 00 f9 f9 f9 f9 f9 f9 f9 01 f9 f9 f9 f9 f9 f9 f9 >> =>0x000080f0c960:[02]f9 f9 f9 f9 f9 f9 f9 00 05 f9 f9 f9 f9 f9 f9 >> 0x000080f0c970: 00 00 00 00 00 00 00 00 00 00 00 00 01 f9 f9 f9 >> 0x000080f0c980: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00 >> 0x000080f0c990: 00 00 00 00 00 04 f9 f9 f9 f9 f9 f9 00 00 00 00 >> 0x000080f0c9a0: 00 02 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 00 00 >> 0x000080f0c9b0: 00 00 00 00 00 00 02 f9 f9 f9 f9 f9 00 00 00 00 >> Shadow byte legend (one shadow byte represents 8 application bytes): >> Addressable: 00 >> Partially addressable: 01 02 03 04 05 06 07 >> Heap left redzone: fa >> Heap right redzone: fb >> Freed heap region: fd >> Stack left redzone: f1 >> Stack mid redzone: f2 >> Stack right redzone: f3 >> Stack partial redzone: f4 >> Stack after return: f5 >> Stack use after scope: f8 >> Global redzone: f9 >> Global init order: f6 >> Poisoned by user: f7 >> Container overflow: fc >> Array cookie: ac >> Intra object redzone: bb >> ASan internal: fe >> Left alloca redzone: ca >> Right alloca redzone: cb >> ==32475==ABORTING >> >> >> >> >> On Tue, Jun 21, 2016 at 11:03 AM Eric Liu via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >>> Author: ioeric >>> Date: Tue Jun 21 12:56:31 2016 >>> New Revision: 273290 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=273290&view=rev >>> Log: >>> Added calculateRangesAfterReplaments() to calculate affacted ranges in >>> the new code. >>> >>> Summary: >>> Added calculateRangesAfterReplaments() to calculate original ranges as >>> well as >>> newly affacted ranges in the new code. >>> >>> Reviewers: klimek, djasper >>> >>> Subscribers: cfe-commits, klimek >>> >>> Differential Revision: http://reviews.llvm.org/D21547 >>> >>> Modified: >>> cfe/trunk/include/clang/Tooling/Core/Replacement.h >>> cfe/trunk/lib/Tooling/Core/Replacement.cpp >>> cfe/trunk/unittests/Tooling/RefactoringTest.cpp >>> >>> Modified: cfe/trunk/include/clang/Tooling/Core/Replacement.h >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Core/Replacement.h?rev=273290&r1=273289&r2=273290&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/include/clang/Tooling/Core/Replacement.h (original) >>> +++ cfe/trunk/include/clang/Tooling/Core/Replacement.h Tue Jun 21 >>> 12:56:31 2016 >>> @@ -57,6 +57,11 @@ public: >>> return RHS.Offset >= Offset && >>> (RHS.Offset + RHS.Length) <= (Offset + Length); >>> } >>> + >>> + /// \brief Whether this range equals to \p RHS or not. >>> + bool operator==(const Range &RHS) const { >>> + return Offset == RHS.getOffset() && Length == RHS.getLength(); >>> + } >>> /// @} >>> >>> private: >>> @@ -222,11 +227,25 @@ bool applyAllReplacements(const std::vec >>> std::string applyAllReplacements(StringRef Code, const Replacements >>> &Replaces); >>> >>> /// \brief Calculates the ranges in a single file that are affected by >>> the >>> -/// Replacements. >>> +/// Replacements. Overlapping ranges will be merged. >>> /// >>> /// \pre Replacements must be for the same file. >>> +/// >>> +/// \returns a non-overlapping and sorted ranges. >>> std::vector<Range> calculateChangedRanges(const Replacements &Replaces); >>> >>> +/// \brief Calculates the new ranges after \p Replaces are applied. >>> These >>> +/// include both the original \p Ranges and the affected ranges of \p >>> Replaces >>> +/// in the new code. >>> +/// >>> +/// \pre Replacements must be for the same file. >>> +/// >>> +/// \return The new ranges after \p Replaces are applied. The new >>> ranges will be >>> +/// sorted and non-overlapping. >>> +std::vector<Range> >>> +calculateRangesAfterReplacements(const Replacements &Replaces, >>> + const std::vector<Range> &Ranges); >>> + >>> /// \brief Groups a random set of replacements by file path. >>> Replacements >>> /// related to the same file entry are put into the same vector. >>> std::map<std::string, Replacements> >>> >>> Modified: cfe/trunk/lib/Tooling/Core/Replacement.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Core/Replacement.cpp?rev=273290&r1=273289&r2=273290&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/Tooling/Core/Replacement.cpp (original) >>> +++ cfe/trunk/lib/Tooling/Core/Replacement.cpp Tue Jun 21 12:56:31 2016 >>> @@ -278,6 +278,30 @@ std::string applyAllReplacements(StringR >>> return Result; >>> } >>> >>> +// Merge and sort overlapping ranges in \p Ranges. >>> +static std::vector<Range> mergeAndSortRanges(std::vector<Range> Ranges) >>> { >>> + std::sort(Ranges.begin(), Ranges.end(), >>> + [](const Range &LHS, const Range &RHS) { >>> + if (LHS.getOffset() != RHS.getOffset()) >>> + return LHS.getOffset() < RHS.getOffset(); >>> + return LHS.getLength() < RHS.getLength(); >>> + }); >>> + std::vector<Range> Result; >>> + for (const auto &R : Ranges) { >>> + if (Result.empty() || >>> + Result.back().getOffset() + Result.back().getLength() < >>> R.getOffset()) { >>> + Result.push_back(R); >>> + } else { >>> + unsigned NewEnd = >>> + std::max(Result.back().getOffset() + >>> Result.back().getLength(), >>> + R.getOffset() + R.getLength()); >>> + Result[Result.size() - 1] = >>> + Range(Result.back().getOffset(), NewEnd - >>> Result.back().getOffset()); >>> + } >>> + } >>> + return Result; >>> +} >>> + >>> std::vector<Range> calculateChangedRanges(const Replacements &Replaces) >>> { >>> std::vector<Range> ChangedRanges; >>> int Shift = 0; >>> @@ -287,7 +311,20 @@ std::vector<Range> calculateChangedRange >>> Shift += Length - R.getLength(); >>> ChangedRanges.push_back(Range(Offset, Length)); >>> } >>> - return ChangedRanges; >>> + return mergeAndSortRanges(ChangedRanges); >>> +} >>> + >>> +std::vector<Range> >>> +calculateRangesAfterReplacements(const Replacements &Replaces, >>> + const std::vector<Range> &Ranges) { >>> + auto MergedRanges = mergeAndSortRanges(Ranges); >>> + tooling::Replacements FakeReplaces; >>> + for (const auto &R : MergedRanges) >>> + FakeReplaces.insert(Replacement(Replaces.begin()->getFilePath(), >>> + R.getOffset(), R.getLength(), >>> + std::string(" ", R.getLength()))); >>> + tooling::Replacements NewReplaces = mergeReplacements(FakeReplaces, >>> Replaces); >>> + return calculateChangedRanges(NewReplaces); >>> } >>> >>> namespace { >>> @@ -434,4 +471,3 @@ Replacements mergeReplacements(const Rep >>> >>> } // end namespace tooling >>> } // end namespace clang >>> - >>> >>> Modified: cfe/trunk/unittests/Tooling/RefactoringTest.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/RefactoringTest.cpp?rev=273290&r1=273289&r2=273290&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/unittests/Tooling/RefactoringTest.cpp (original) >>> +++ cfe/trunk/unittests/Tooling/RefactoringTest.cpp Tue Jun 21 12:56:31 >>> 2016 >>> @@ -462,13 +462,96 @@ TEST(Range, CalculateRangesOfReplacement >>> >>> std::vector<Range> Ranges = calculateChangedRanges(Replaces); >>> >>> - EXPECT_EQ(3ul, Ranges.size()); >>> + EXPECT_EQ(2ul, Ranges.size()); >>> EXPECT_TRUE(Ranges[0].getOffset() == 0); >>> EXPECT_TRUE(Ranges[0].getLength() == 0); >>> EXPECT_TRUE(Ranges[1].getOffset() == 6); >>> - EXPECT_TRUE(Ranges[1].getLength() == 6); >>> - EXPECT_TRUE(Ranges[2].getOffset() == 12); >>> - EXPECT_TRUE(Ranges[2].getLength() == 16); >>> + EXPECT_TRUE(Ranges[1].getLength() == 22); >>> +} >>> + >>> +TEST(Range, RangesAfterReplacements) { >>> + std::vector<Range> Ranges = {Range(5, 2), Range(10, 5)}; >>> + Replacements Replaces = {Replacement("foo", 0, 2, "1234")}; >>> + std::vector<Range> Expected = {Range(0, 4), Range(7, 2), Range(12, >>> 5)}; >>> + EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, >>> Ranges)); >>> +} >>> + >>> +TEST(Range, RangesBeforeReplacements) { >>> + std::vector<Range> Ranges = {Range(5, 2), Range(10, 5)}; >>> + Replacements Replaces = {Replacement("foo", 20, 2, "1234")}; >>> + std::vector<Range> Expected = {Range(5, 2), Range(10, 5), Range(20, >>> 4)}; >>> + EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, >>> Ranges)); >>> +} >>> + >>> +TEST(Range, NotAffectedByReplacements) { >>> + std::vector<Range> Ranges = {Range(0, 2), Range(5, 2), Range(10, 5)}; >>> + Replacements Replaces = {Replacement("foo", 3, 2, "12"), >>> + Replacement("foo", 12, 2, "12"), >>> + Replacement("foo", 20, 5, "")}; >>> + std::vector<Range> Expected = {Range(0, 2), Range(3, 4), Range(10, 5), >>> + Range(20, 0)}; >>> + EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, >>> Ranges)); >>> +} >>> + >>> +TEST(Range, RangesWithNonOverlappingReplacements) { >>> + std::vector<Range> Ranges = {Range(0, 2), Range(5, 2), Range(10, 5)}; >>> + Replacements Replaces = {Replacement("foo", 3, 1, ""), >>> + Replacement("foo", 6, 1, "123"), >>> + Replacement("foo", 20, 2, "12345")}; >>> + std::vector<Range> Expected = {Range(0, 2), Range(3, 0), Range(4, 4), >>> + Range(11, 5), Range(21, 5)}; >>> + EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, >>> Ranges)); >>> +} >>> + >>> +TEST(Range, RangesWithOverlappingReplacements) { >>> + std::vector<Range> Ranges = {Range(0, 2), Range(5, 2), Range(15, 5), >>> + Range(30, 5)}; >>> + Replacements Replaces = { >>> + Replacement("foo", 1, 3, ""), Replacement("foo", 6, 1, "123"), >>> + Replacement("foo", 13, 3, "1"), Replacement("foo", 25, 15, "")}; >>> + std::vector<Range> Expected = {Range(0, 1), Range(2, 4), Range(12, 5), >>> + Range(22, 0)}; >>> + EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, >>> Ranges)); >>> +} >>> + >>> +TEST(Range, MergeIntoOneRange) { >>> + std::vector<Range> Ranges = {Range(0, 2), Range(5, 2), Range(15, 5)}; >>> + Replacements Replaces = {Replacement("foo", 1, 15, "1234567890")}; >>> + std::vector<Range> Expected = {Range(0, 15)}; >>> + EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, >>> Ranges)); >>> +} >>> + >>> +TEST(Range, ReplacementsStartingAtRangeOffsets) { >>> + std::vector<Range> Ranges = {Range(0, 2), Range(5, 5), Range(15, 5)}; >>> + Replacements Replaces = { >>> + Replacement("foo", 0, 2, "12"), Replacement("foo", 5, 1, "123"), >>> + Replacement("foo", 7, 4, "12345"), Replacement("foo", 15, 10, >>> "12")}; >>> + std::vector<Range> Expected = {Range(0, 2), Range(5, 9), Range(18, >>> 2)}; >>> + EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, >>> Ranges)); >>> +} >>> + >>> +TEST(Range, ReplacementsEndingAtRangeEnds) { >>> + std::vector<Range> Ranges = {Range(0, 2), Range(5, 2), Range(15, 5)}; >>> + Replacements Replaces = {Replacement("foo", 6, 1, "123"), >>> + Replacement("foo", 17, 3, "12")}; >>> + std::vector<Range> Expected = {Range(0, 2), Range(5, 4), Range(17, >>> 4)}; >>> + EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, >>> Ranges)); >>> +} >>> + >>> +TEST(Range, AjacentReplacements) { >>> + std::vector<Range> Ranges = {Range(0, 0), Range(15, 5)}; >>> + Replacements Replaces = {Replacement("foo", 1, 2, "123"), >>> + Replacement("foo", 12, 3, "1234")}; >>> + std::vector<Range> Expected = {Range(0, 0), Range(1, 3), Range(13, >>> 9)}; >>> + EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, >>> Ranges)); >>> +} >>> + >>> +TEST(Range, MergeRangesAfterReplacements) { >>> + std::vector<Range> Ranges = {Range(8, 0), Range(5, 2), Range(9, 0), >>> Range(0, 1)}; >>> + Replacements Replaces = {Replacement("foo", 1, 3, ""), >>> + Replacement("foo", 7, 0, "12"), >>> Replacement("foo", 9, 2, "")}; >>> + std::vector<Range> Expected = {Range(0, 1), Range(2, 4), Range(7, 0), >>> Range(8, 0)}; >>> + EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, >>> Ranges)); >>> } >>> >>> TEST(DeduplicateTest, removesDuplicates) { >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> cfe-commits@lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> >> -- >> Mike >> Sent from phone >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
