[Libreoffice-commits] core.git: Branch 'distro/collabora/co-2021' - svx/source

Stephan Bergmann (via logerrit) logerrit at kemper.freedesktop.org
Wed Jun 16 09:33:19 UTC 2021


 svx/source/svdraw/svdetc.cxx |   80 ++++++++++++++-----------------------------
 1 file changed, 27 insertions(+), 53 deletions(-)

New commits:
commit 94c654295e1d0c91fa97bd91d688c63c29ab965d
Author:     Stephan Bergmann <sbergman at redhat.com>
AuthorDate: Tue Jun 15 17:57:56 2021 +0200
Commit:     Mike Kaganski <mike.kaganski at collabora.com>
CommitDate: Wed Jun 16 11:32:46 2021 +0200

    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 at redhat.com>
    (cherry picked from commit 541f94df85756d3a383b1f9ba49841ca0011b52e)
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/117293
    Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice at gmail.com>
    Reviewed-by: Mike Kaganski <mike.kaganski at collabora.com>

diff --git a/svx/source/svdraw/svdetc.cxx b/svx/source/svdraw/svdetc.cxx
index 0d82f6632b35..53303e0b7b0d 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>
@@ -371,63 +373,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;
 }
 


More information about the Libreoffice-commits mailing list