[Libreoffice-commits] core.git: svx/source
Stephan Bergmann (via logerrit)
logerrit at kemper.freedesktop.org
Tue Jun 15 18:38:24 UTC 2021
svx/source/svdraw/svdetc.cxx | 80 ++++++++++++++-----------------------------
1 file changed, 27 insertions(+), 53 deletions(-)
New commits:
commit 541f94df85756d3a383b1f9ba49841ca0011b52e
Author: Stephan Bergmann <sbergman at redhat.com>
AuthorDate: Tue Jun 15 17:57:56 2021 +0200
Commit: Stephan Bergmann <sbergman at redhat.com>
CommitDate: Tue Jun 15 20:37:42 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>
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;
}
More information about the Libreoffice-commits
mailing list