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
