memcpy-param-overlap

At least UITest_impress_tests started to fail at

> ==608818==ERROR: AddressSanitizer: memcpy-param-overlap: memory ranges [0x6020000ef270,0x6020000ef276) and [0x6020000ef274, 0x6020000ef27a) overlap
>  #0 in __asan_memcpy at ~/github.com/llvm/llvm-project/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:22:3
>  #1 in RemoveWhichRange(unsigned short const*, unsigned short, unsigned short) at svx/source/svdraw/svdetc.cxx:413:17
>  #2 in SdrObjEditView::SetAttributes(SfxItemSet const&, bool) at svx/source/svdraw/svdedxv.cxx:2222:19
>  #3 in SdrCreateView::SetAttributes(SfxItemSet const&, bool) at svx/source/svdraw/svdcrtv.cxx:883:29
>  #4 in SdrView::SetAttributes(SfxItemSet const&, bool) at include/svx/svdview.hxx:193:96
>  #5 in sd::View::SetAttributes(SfxItemSet const&, bool, bool, bool) at sd/source/ui/view/sdview.cxx:488:28
>  #6 in sd::DrawView::SetAttributes(SfxItemSet const&, bool, bool, bool) at sd/source/ui/view/drawview.cxx:288:27
>  #7 in sd::TextObjectBar::Execute(SfxRequest&) at sd/source/ui/view/drtxtob1.cxx:819:21
>  #8 in SfxStubTextObjectBarExecute(SfxShell*, SfxRequest&) at workdir/SdiTarget/sd/sdi/sdslots.hxx:17883:1
[...]

(followed by

> ==647521==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000258856 at pc 0x0000002f7b0a bp 0x7f7f7a69fdb0 sp 0x7f7f7a69f560
> READ of size 6 at 0x602000258856 thread T9 (cppu_threadpool)
>  #0 in __asan_memmove at /data/sbergman/github.com/llvm/llvm-project/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:30:3
>  #1 in RemoveWhichRange(unsigned short const*, unsigned short, unsigned short) at svx/source/svdraw/svdetc.cxx:413:17
>  #2 in SdrObjEditView::SetAttributes(SfxItemSet const&, bool) at svx/source/svdraw/svdedxv.cxx:2222:19
>  #3 in SdrCreateView::SetAttributes(SfxItemSet const&, bool) at svx/source/svdraw/svdcrtv.cxx:883:29
>  #4 in SdrView::SetAttributes(SfxItemSet const&, bool) at include/svx/svdview.hxx:193:96
>  #5 in sd::View::SetAttributes(SfxItemSet const&, bool, bool, bool) at sd/source/ui/view/sdview.cxx:488:28
>  #6 in sd::DrawView::SetAttributes(SfxItemSet const&, bool, bool, bool) at sd/source/ui/view/drawview.cxx:288:27
>  #7 in sd::TextObjectBar::Execute(SfxRequest&) at sd/source/ui/view/drtxtob1.cxx:819:21
>  #8 in SfxStubTextObjectBarExecute(SfxShell*, SfxRequest&) at workdir/SdiTarget/sd/sdi/sdslots.hxx:17883:1
[...]
> 0x602000258856 is located 0 bytes to the right of 6-byte region [0x602000258850,0x602000258856)
> allocated by thread T9 (cppu_threadpool) here:
[...]

if the memcpy were replaced with memmove) after
8aaa28ed43978a9a4a20d62368410a57ec05c23f "Assert on valid order of which ids in
ranges on SfxItemSet creation":

Where in the past it had called

RemoveWhichRange({10951, 10951, 4007, 4007, 0}, 4007, 4062)

and

RemoveWhichRange({10950, 10950, 4007, 4007, 0}, 4007, 4062)

on wrongly sorted ranges, for which the implementation of RemoveWhichRange
happened to work, it now calls

RemoveWhichRange({4007, 4007, 10951, 10951, 0}, 4007, 4062)

and

RemoveWhichRange({4007, 4007, 10950, 10950, 0}, 4007, 4062)

on correctly sorted ranges, which uncovered the broken implementation.

Given that RemoveWhichRange is presumably not hot (e.g., being called just two
times during UITest_impress_tests), turn it into a less sophisticated, but also
likely less error-prone algorithm.

(Leaving unit tests as a TODO, given that RemoveWhichRange is not
currently exported from Library_svxcore, and the existing CppunitTest_svx_unit
links against Library_svxcore rather than using
gb_CppunitTest_use_library_objects.)

Change-Id: I8a1c1d5db8073928ad2f6e88d581f24a0e882925
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/117264
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

1 files changed, 27 insertions, 53 deletions

diff --git a/svx/source/svdraw/svdetc.cxx b/svx/source/svdraw/svdetc.cxx
index 69d0306c77b7..9967376bf078 100644
--- a/svx/source/svdraw/svdetc.cxx
+++ b/svx/source/svdraw/svdetc.cxx
@@ -19,6 +19,8 @@
 
 #include <sal/config.h>
 
+#include <algorithm>
+
 #include <officecfg/Office/Common.hxx>
 #include <svtools/colorcfg.hxx>
 #include <svx/svdetc.hxx>
@@ -368,63 +370,35 @@ bool SearchOutlinerItems(const SfxItemSet& rSet, bool bInklDefaults, bool* pbOnl
 std::unique_ptr<sal_uInt16[]> RemoveWhichRange(const sal_uInt16* pOldWhichTable, sal_uInt16 nRangeBeg, sal_uInt16 nRangeEnd)
 {
     // Six possible cases (per range):
-    //         [Beg..End]          Range, to delete
+    //         [Beg..End]          [nRangeBeg, nRangeEnd], to delete
     // [b..e]    [b..e]    [b..e]  Cases 1,3,2: doesn't matter, delete, doesn't matter  + Ranges
     // [b........e]  [b........e]  Cases 4,5  : shrink range                            | in
     // [b......................e]  Case  6    : splitting                               + pOldWhichTable
-    sal_uInt16 nCount=0;
-    while (pOldWhichTable[nCount]!=0) nCount++;
-    nCount++; // nCount should now be an odd number (0 for end of array)
-    DBG_ASSERT((nCount&1)==1,"RemoveWhichRange: WhichTable doesn't have an odd number of entries.");
-    sal_uInt16 nAlloc=nCount;
-    // check necessary size of new array
-    sal_uInt16 nNum=nCount-1;
-    while (nNum!=0) {
-        nNum-=2;
-        sal_uInt16 nBeg=pOldWhichTable[nNum];
-        sal_uInt16 nEnd=pOldWhichTable[nNum+1];
-        if (nEnd<nRangeBeg)  /*nCase=1*/ ;
-        else if (nBeg>nRangeEnd) /* nCase=2 */ ;
-        else if (nBeg>=nRangeBeg && nEnd<=nRangeEnd) /* nCase=3 */ nAlloc-=2;
-        else if (nEnd<=nRangeEnd) /* nCase=4 */;
-        else if (nBeg>=nRangeBeg) /* nCase=5*/ ;
-        else /* nCase=6 */ nAlloc+=2;
-    }
-
-    std::unique_ptr<sal_uInt16[]> pNewWhichTable(new sal_uInt16[nAlloc]);
-    memcpy(pNewWhichTable.get(), pOldWhichTable, nAlloc*sizeof(sal_uInt16));
-    pNewWhichTable[nAlloc-1]=0; // in case 3, there's no 0 at the end.
-    // now remove the unwanted ranges
-    nNum=nAlloc-1;
-    while (nNum!=0) {
-        nNum-=2;
-        sal_uInt16 nBeg=pNewWhichTable[nNum];
-        sal_uInt16 nEnd=pNewWhichTable[nNum+1];
-        unsigned nCase=0;
-        if (nEnd<nRangeBeg) nCase=1;
-        else if (nBeg>nRangeEnd) nCase=2;
-        else if (nBeg>=nRangeBeg && nEnd<=nRangeEnd) nCase=3;
-        else if (nEnd<=nRangeEnd) nCase=4;
-        else if (nBeg>=nRangeBeg) nCase=5;
-        else nCase=6;
-        switch (nCase) {
-            case 3: {
-                unsigned nTailBytes=(nCount-(nNum+2))*sizeof(sal_uInt16);
-                memcpy(&pNewWhichTable[nNum],&pNewWhichTable[nNum+2],nTailBytes);
-                nCount-=2; // remember: array is now smaller
-            } break;
-            case 4: pNewWhichTable[nNum+1]=nRangeBeg-1; break;
-            case 5: pNewWhichTable[nNum]=nRangeEnd+1;     break;
-            case 6: {
-                unsigned nTailBytes=(nCount-(nNum+2))*sizeof(sal_uInt16);
-                memcpy(&pNewWhichTable[nNum+4],&pNewWhichTable[nNum+2],nTailBytes);
-                nCount+=2; // remember:array is now larger
-                pNewWhichTable[nNum+2]=nRangeEnd+1;
-                pNewWhichTable[nNum+3]=pNewWhichTable[nNum+1];
-                pNewWhichTable[nNum+1]=nRangeBeg-1;
-            } break;
-        } // switch
+    std::vector<sal_uInt16> buf;
+    for (auto p = pOldWhichTable; *p != 0; p += 2) {
+        auto const begin = p[0];
+        auto const end = p[1];
+        if (end < nRangeBeg || begin > nRangeEnd) { // cases 1, 2
+            buf.push_back(begin);
+            buf.push_back(end);
+        } else if (begin >= nRangeBeg && end <= nRangeEnd) { // case 3
+            // drop
+        } else if (end <= nRangeEnd) { // case 4
+            buf.push_back(begin);
+            buf.push_back(nRangeBeg - 1);
+        } else if (begin >= nRangeBeg) { // case 5
+            buf.push_back(nRangeEnd + 1);
+            buf.push_back(end);
+        } else { // case 6
+            buf.push_back(begin);
+            buf.push_back(nRangeBeg - 1);
+            buf.push_back(nRangeEnd + 1);
+            buf.push_back(end);
+        }
     }
+    std::unique_ptr<sal_uInt16[]> pNewWhichTable(new sal_uInt16[buf.size() + 1]);
+    std::copy(buf.begin(), buf.end(), pNewWhichTable.get());
+    pNewWhichTable[buf.size()] = 0;
     return pNewWhichTable;
 }