Hi, I am a new contributor and would like to submit a small patch for LibreOffice Impress.
I attempted to push via Gerrit (karekaa) but keep getting HTTP 403 on refs/for/master, despite my account being set up with an SSH key and confirmed email. I suspect this is related to the refs/for/refs/head/* ACL typo (missing s) in the Dev-ACL-Template project. Sending the patch by email in the meantime. What the patch does: ParagraphTarget.Paragraph is declared as short (sal_Int16) in the UNO IDL, but Outliner::GetAbsPos() returns sal_Int32. The two cast sites in SdPage::onParagraphInserted() and SdPage::onParagraphRemoving() in sd/source/core/sdpage_animations.cxx silently truncate the paragraph index for any shape with more than 32,767 paragraphs. The FIXME comment acknowledging this has been there since at least 2003. This patch: - Replaces the silent static_cast<sal_Int16> with an explicit nAbsPos variable and a SAL_WARN_IF guard, so overflow is visible in debug logs without breaking the IDL ABI - Adds a CppUnit regression test (testParagraphTargetIndexPreserved in sd/qa/unit/misc-tests.cxx) that imports a PPTX file with six per-paragraph Fly-In entrance animations and asserts that ParagraphTarget.Paragraph indices 0–5 are preserved correctly The patch applies cleanly to current master (aa14545afd2d). Patch attached: 0001-sd-warn-on-ParagraphTarget.Paragraph-sal_Int16-overf.patch Best regards, Kjell Arne Rekaa, Oslo, Norway Phone: +47-90 88 99 06
From 842a1dc856a89775d6abef68eaa05cc0ac833598 Mon Sep 17 00:00:00 2001 From: Kjell Arne Rekaa <[email protected]> Date: Sun, 21 Jun 2026 03:02:13 +0200 Subject: [PATCH] sd: warn on ParagraphTarget.Paragraph sal_Int16 overflow ParagraphTarget.Paragraph is declared as 'short' (sal_Int16) in the IDL, but GetAbsPos() returns sal_Int32. The two cast sites in onParagraphInserted and onParagraphRemoving silently truncated indices above 32767. Replace the bare static_cast with an explicit nAbsPos variable and a SAL_WARN_IF guard so overflow is visible in debug logs without changing the IDL ABI. Add a CppUnit test that imports a PPTX with six per-paragraph Fly-In animations and asserts that ParagraphTarget.Paragraph indices 0..5 are preserved correctly after import. Change-Id: I09110b12ed2c19ef9ac97b182023b8b7069cc93a --- sd/qa/unit/data/para-anim-fly-in.pptx | Bin 0 -> 4625 bytes sd/qa/unit/misc-tests.cxx | 51 ++++++++++++++++++++++++++ sd/source/core/sdpage_animations.cxx | 20 +++++++--- 3 files changed, 65 insertions(+), 6 deletions(-) create mode 100644 sd/qa/unit/data/para-anim-fly-in.pptx diff --git a/sd/qa/unit/data/para-anim-fly-in.pptx b/sd/qa/unit/data/para-anim-fly-in.pptx new file mode 100644 index 0000000000000000000000000000000000000000..d7bba1f663b13b6f72097810f04d6f32725e72f4 GIT binary patch literal 4625 zcmdUyc{o(>`^V4Nw?ZUK))2;?Xu*V1$QZj4nlje0#8}2yM&e^>VzQR(dzr{yc9TM6 zjfPN`WG5l}_fq-(m|vgY|G)cO*E#38u5;ekdG6=+yzg_vbSSCV0RW%_#5g|Oxc`Jz z`Zx#xTqyy74FCXh4Hst=!Wm^{gmy)^TS$95Axonxp_FngmoPOAZaj@pF&6bot@?}+ z<9p1UXj?W$3=%Ec`E_33Q;q7P0CiqI*eV6XZMq~)Q(E7ks!zzVNk95f2bY5&SaEob z>JZGZ+;m}uz2?>`DM`i=PMRhbOs`8V!^XglB!VnOVhsKjXTOmUU7}H(Vh|=+q<^V- ztl`zgyTZ3gazAFS=1tL<o6+i*CkY&qrlxfCP(gXQmxeqE@)A(R55kt5>^4<zF@)3* z!71pT#?;v~{NVT(Tziflk7T1#ZgN`FiHrHAZP$>}O;@`5+U5)S?->sG{G;nO9V%?Y z&q0j06Y}QXnkCjNH4){uT+)(#@?WbUyRUmgo@m<L<bSpdNSf{5JKhYiGcMbaEmndZ zNai_`p>R0>0CFGzU<3fb$_;^ZmyzB*<J#-BU(2$*S;VB=N~qNB3kp>jjG)8Bt^<Ne zW%bsg)|!(w)t{S>G7s%+s@5#}#xSMyD9~e+mnkXk#E2|@U3Bt)iMa@4thy(8m9HSv zJNeaOtXrXQKISRI<>q`ouj2KwCvTFpJ1iv@^}kCHsJbotzdD4@!%uiYn3$h$DY#An z&Sn*od;^HHf~|epJ6{kMhMBfu{J|f$&DM+sTGO1Y`5aH1@`%6i>)X<of0f5G*w#JC zcyN9~8d4-$5CE`H0KhInuC6E<S2u(^!Wjie*}FLJ(t_($gA!#~s5|_i@V4Z0^!9Z6 zPn=l{M9e_6g}2H}ts|u(17o@~;%F#Pomp>pAUK6V2UJ}3<druW*jsqY?lay9SMcZY zwe6=MO|A5Yy%`*ZeN@j>r`37VMR8i9qj=nVp;EOv>8uVnC=q&Ii;-X9!Fy+4lzeBy zTd9=QC*z3ZzE4^~Q7_m|$iR(a=4r|&g1y5&EAgEAb{<l74mTMQ*3Wk=BZ;2M?~U29 z>qS%g23>aud}7PBCtJlp2>n5$HV=!_b+<4HuS+EYS9y0#cN8c3$0Vn@V4Fa|Q$2np zC-o-|<JgI_g>acv=eV{n^?GtYw7SL5Z1DyeMy!(5Qid~js17Es#(u}I1pt6r5CHK1 zN!)KZ$kX==7DCi7SVHDE*x_>nB^VZbHii|>Y?-NA7j<=@P)pRhsLKzdb}3)Xd*KWO zGQ1<VbqA+b4P$L@zR*{aDt66NwT&kE?LS*mVtEP2X`t4hwJF}h`yl#tQ)in;igQng z2-Cz}UrOq|bu3)$kgc{O6I*<!F}HN_d%N~C(W)i0IB6>`*_oLBn;)9F#7_1Fau+<> zzB^(6J#LO<DU`KN43GQF8_4~l)#y-F62E<4z8rS@!|1|1U!+)Ui&EmMYxhru>E*BU zMDW2h%;CM*WvKwbM;HK({z-#7(%u^3{`(@g3()NKVZG5aEdCP(g`57Fx^OS!(-ypy zk}{zhI`c!t&6%aH$lJ`MAGHvYmYqKbOzDV3)dl58T<yN6mgEe~eA^76@*N>stR0h+ z_l~gO?>{o5{W1I@vbF|xOP2jkQx+EMT;EafE;{%z4_~zMctfl41*K4jIkag11@6a& zc^AgS^)jIv3ONP^Dp~c|I+jQ5;;Wm@W6|*;S3B4YUeQ~Pi1vPQ5$)aL`=O)UBn)Tu zjqkk8K*6ZQUBTFjOpr&R)tl7YGS2f2T=tT6$O46Du!YWOKRfU7415jl9zE#LFoK1O zv%(obS(fG*3dojACee~r$&4HhHPRvxuE{6QmDVC2Mk%MVX_u|!*dn4%gfYNNqBRo( zd{7x}h{!Tii_f#tgTZhk@DI_$Qo@@IVMoWj1()o3@!I9u?{Xcv26=?WVof+MwKu}- z6=t~;jB`C4ZuN)oE@XEXldKKu3{;e`n=^W%(0U4#=iCF2gvUd%ir&=nA}fNa+(MbX z<_Q)&lKB#`HVfIj=e{13A1eE_^Kh{YUmooi<W(`=5eLQ;2XVy5Hw@vVDJ)*UxQr38 z$|8I@Jq7s)UKaGjt@D0LR9WYk5iO(M9IwDEqrGNL4FaX;bsm{P7Ae9VgGxJL@CX)@ z^z@*g`m`}&oJT}CDMYxLLbxd<1SoaGqU7?+7%0Lg^~CXIZ>@>mRZp!drtf$Q;MqGf zdIbt^@;5Eci>1<hPF6g`jbBs2&kDVu!q77F(guM_<?PaB#bwTy6JuoGQ@+A0XAj<6 zNMwJpjBLva)nZcqq}U*t3(p~~$lWuwiW-FF-~h*{O`D3BAEM;e?0j5F{M*(6Ju;!y zCQE@{!9fwa+40Nwo~kRcErs~9&sM+Dsgc~?Cg3@ZQLSBf3>P)GZm{XNJG+N^adFI7 zca^Z5b=jC>WL?%xP!GG|af4gODTnLS%&}3X^4@{&ck6JLLboVh31V&<syTgIW=xn7 zePJ-E%0V%Pa(rQ>hbSs!=JQCHMNo~Gf!Hx?ezoGdz&EU@@4$erb#8KT;5DC9NBwAA zP0LQ+@R_I^fY8ltX``|Srn%<fR=hI28(U92{bh4hG#F?NNjbMOirPmFNF~V4N!r-u zvC|Z>eq4zyhY8;{Qss9RmXX#HOOa0CrOQJtn?;LT%o5Yh)9r?jAnPWcc9(|fW2@H) zt&lQ8D?b{X7>k2!q!uMtX{Hu+mKSM&*Hv?oD__ftNH3fa#rX1~;-bZN)!h7(1rR+% z@$V;~xif7ph~nD=EjA}y5EF>elmOT@J<Ag?(l>Kb+EEp53CuNRjn3ScT~VcLfIL$! zd)$?8502m9y0}(w*x>bCn)8r{Gi3honcBePDejg^^MK9gv>>D5oFSX9hc<7-8%@o@ z{FS`6(N&xL2Q3CG-;-Mj0KhpA00<nknBRu`%Wr-e&7Zpb!r`?n3suLE)e9tlGn7@m z&{vo4cqYSpS~`2Di0BgaXN)VuIVu|ACB%ggE*VVtf`&1LZBWn@tJwJ!!2+7viO3vN zDtko?&)MkuASxL{C4c=ADc3kD_CS?JNt*KBmc-r>MQs!IV=b6)#y;yrvFFdEdk1H< z*I&+Cq5O6WGgR_Mb-m9rWrHvKI&=HGBgd<YD^Iq}Mjn-t^Ik8Z9p-63PHVTN8bq$; zDdehL>M>NB>K>mwc5vc1eKsl-KmcGz0RV#gCw>)<cJV;{aoFF3U(?4ueCJFQV7auq z&7iPBlN0f@XvDg+FWX5-WD5v?t0=)l7wWj<7d+c!VU0f&`7Y^{yNF`tkX>&YbBh7} zwDDmkt$uBN!-l@e<vCWf(7O_8QgC`onvYJ!IPl%)Lyb4v@ynWqVmIiW?mz#m+{A&S z15Zc!WH?UaLg5HMV$xeI`z52e`(Ibl+jTe?6<4C%2*=g&#)9;(=jw;m3q1(Js^)0L zD|Z^adq!`j8A}UW9zPzpK%m~LQ@o8FWkC0QP|Wf=8JPg>sYZLDXj7!W7oYdeetM+0 z?h2<Q=;x;Fz)vFuzMnha^VHk}!_o_?wJHgYz41|$JY6l(A%50wN5<1zJOdiR1My&I zfBU-lY<sGjd!z0{XJ4)GH_TSvQ9Pl3aYsQLs(&i@nk1>|r7wP#jJ1;3=<q$(B>$H3 zeZ=icJUMrQWs%lazqmV7A*8$DS0_YpV$}kdXfJGSmD*mxdvXrRj-vJSw+v$INDZkN zSPbt*lqqBR+V}4?+T1pg_*2&^qU+F8Z)CzhQrMEzSxa*g8XJ1Aqm?v2Y0_VEPpitQ zaMk^I(cMB<Z50Q8WH?;=cpX{SBkpnY!w2(RQwa9)eDF4Lsfg+_|BU~U@s+pXcur-% zbcLs>Hu>PwjMpI(|8%9Uve#c7v^yN1Lwk_BwdX7@;UEA|r6l7{7w(Qixc%Yo_u#+W z4an|t7hXT(c)e&O`)(d`ogu0O^w1yBFgU_mb3y+3<|?(6oHL_L(HACW+qs3ipN6k$ z64gR-pJbU4l<2zLPR8(xZTqaRf8iDswW02QuN{7wYcbn=){m#O!d9*si0)J@OCCvh z=t3Xn8xzu4y3_^LT$SdG8|e=Y{3(!85+*X9-<dgt^n5LgxI-Pmgo(UQH&b3jL<BGJ zQTMHeziWR`%WdB+$(E^VA^D)?@{9?98UB7*>J_pNLM`rY!#sR9;0Lpy%g;!n1|TA4 zLKnxS#oFvv7=wQ~8531IFagqFy*=AQ8=WzYNf2{pd4Lvq)GQ845ri13ZdbwHy_Grk zREz7vdyS=|O{C-3Hl=$#Ds1_Z{|Qfr9#wxV5k8Wo?#(yVx_dZKy?I%ZmtG+p+!I{& z0m(?>=wk0;cQbA>7OYhFq{}c_2_6v__yG+_QEQHp<V)x9n)zngO{j>-+lYTMM0ox< zvTfP3&sUw!N%V_}(HTf@!0fZ&HvZZT%7Y8J-#HJ^ytk11Xx_J+<TU^Pb5RflJLSJO z#Cw4S?cV*RJ%*79kT-#Qfd%P;4zz^iFOqjxdw~T>QT)BvB3B@9Gxh=tvZ4H~@P9NN z<YMGM>wAF(IZ^!<`(MBA<P!Ul+AYR=fdzHY9gx_E*8h}day9Y_w-;DYGyOibf2()o za^&v47g&%o<6q?VeImJ+lPi;3;$C1uYE1i-_x9DVH169`<Z}D7xVtg;0t;d|{1>@> cAG%NU*EWUeP}A(b%>Xa}GZX;8%)C4L4^7CZhX4Qo literal 0 HcmV?d00001 diff --git a/sd/qa/unit/misc-tests.cxx b/sd/qa/unit/misc-tests.cxx index 538e6fdf5e16..b88363137892 100644 --- a/sd/qa/unit/misc-tests.cxx +++ b/sd/qa/unit/misc-tests.cxx @@ -65,6 +65,11 @@ #include <editeng/editview.hxx> #include <editeng/outliner.hxx> #include <svx/svdotext.hxx> +#include <com/sun/star/animations/XAnimationNodeSupplier.hpp> +#include <com/sun/star/animations/XAnimate.hpp> +#include <com/sun/star/presentation/ParagraphTarget.hpp> +#include <animations/animationnodehelper.hxx> +#include <algorithm> using namespace ::com::sun::star; @@ -107,6 +112,7 @@ public: void testDuplicateAndMove(); void testApiXDrawPageDuplicator(); void testApiXMasterPagesSupplier(); + void testParagraphTargetIndexPreserved(); CPPUNIT_TEST_SUITE(SdMiscTest); CPPUNIT_TEST(testTdf99396_UndoCellVerticalAlignment); @@ -139,6 +145,7 @@ public: CPPUNIT_TEST(testDuplicateAndMove); CPPUNIT_TEST(testApiXDrawPageDuplicator); CPPUNIT_TEST(testApiXMasterPagesSupplier); + CPPUNIT_TEST(testParagraphTargetIndexPreserved); CPPUNIT_TEST_SUITE_END(); }; @@ -1367,6 +1374,50 @@ void SdMiscTest::testApiXMasterPagesSupplier() CPPUNIT_ASSERT(xMasterPages->getCount() >= 1); } +// Test that ParagraphTarget.Paragraph indices are preserved correctly when importing +// per-paragraph animation effects from PPTX. The file has a text shape with 6 paragraphs +// (En, To, Tre, Fire, Fem, Seks) and individual Fly-In entrance animations on each. +// ParagraphTarget.Paragraph is declared as 'short' (sal_Int16) in the IDL but the PPTX +// import path (oox) and the sd core path (sdpage_animations.cxx) both cast from sal_Int32. +void SdMiscTest::testParagraphTargetIndexPreserved() +{ + createSdImpressDoc("para-anim-fly-in.pptx"); + + uno::Reference<drawing::XDrawPagesSupplier> xDoc(mxComponent, uno::UNO_QUERY_THROW); + uno::Reference<drawing::XDrawPage> xPage(xDoc->getDrawPages()->getByIndex(0), + uno::UNO_QUERY_THROW); + uno::Reference<animations::XAnimationNodeSupplier> xSupplier(xPage, uno::UNO_QUERY_THROW); + + std::vector<uno::Reference<animations::XAnimationNode>> aNodes; + anim::create_deep_vector(xSupplier->getAnimationNode(), aNodes); + + std::vector<sal_Int16> aFoundIndices; + for (const auto& xNode : aNodes) + { + uno::Reference<animations::XAnimate> xAnim(xNode, uno::UNO_QUERY); + if (!xAnim.is()) + continue; + presentation::ParagraphTarget aTarget; + if (xAnim->getTarget() >>= aTarget) + aFoundIndices.push_back(aTarget.Paragraph); + } + + // Each of the 6 paragraphs should have at least one animation effect referencing it. + // Collect unique indices and verify they are 0..5. + std::sort(aFoundIndices.begin(), aFoundIndices.end()); + aFoundIndices.erase(std::unique(aFoundIndices.begin(), aFoundIndices.end()), + aFoundIndices.end()); + + CPPUNIT_ASSERT_EQUAL_MESSAGE("Expected animation effects for 6 distinct paragraphs", + static_cast<size_t>(6), aFoundIndices.size()); + for (sal_Int16 i = 0; i < 6; ++i) + { + CPPUNIT_ASSERT_EQUAL_MESSAGE( + "ParagraphTarget.Paragraph index was truncated or wrong", + i, aFoundIndices[static_cast<size_t>(i)]); + } +} + CPPUNIT_TEST_SUITE_REGISTRATION(SdMiscTest); CPPUNIT_PLUGIN_IMPLEMENT(); diff --git a/sd/source/core/sdpage_animations.cxx b/sd/source/core/sdpage_animations.cxx index 3cf35bbd4f02..347be72bda49 100644 --- a/sd/source/core/sdpage_animations.cxx +++ b/sd/source/core/sdpage_animations.cxx @@ -26,6 +26,8 @@ #include <CustomAnimationEffect.hxx> #include <sdpage.hxx> #include <EffectMigration.hxx> +#include <sal/log.hxx> +#include <limits> using namespace ::sd; using namespace ::com::sun::star::uno; @@ -112,9 +114,12 @@ void SdPage::onParagraphInserted( const ::Outliner* pOutliner, Paragraph const * { ParagraphTarget aTarget; aTarget.Shape.set( pObj->getUnoShape(), UNO_QUERY ); - /* FIXME: Paragraph should be sal_Int32, though more than 64k - * paragraphs at a shape are unlikely... */ - aTarget.Paragraph = static_cast<sal_Int16>(pOutliner->GetAbsPos( pPara )); + const sal_Int32 nAbsPos = pOutliner->GetAbsPos( pPara ); + SAL_WARN_IF( nAbsPos > std::numeric_limits<sal_Int16>::max(), + "sd.core", + "SdPage::onParagraphInserted: paragraph index " << nAbsPos + << " exceeds sal_Int16 range; ParagraphTarget.Paragraph will overflow" ); + aTarget.Paragraph = static_cast<sal_Int16>(nAbsPos); getMainSequence()->insertTextRange( Any( aTarget ) ); } @@ -127,9 +132,12 @@ void SdPage::onParagraphRemoving( const ::Outliner* pOutliner, Paragraph const * { ParagraphTarget aTarget; aTarget.Shape.set( pObj->getUnoShape(), UNO_QUERY ); - /* FIXME: Paragraph should be sal_Int32, though more than 64k - * paragraphs at a shape are unlikely... */ - aTarget.Paragraph = static_cast<sal_Int16>(pOutliner->GetAbsPos( pPara )); + const sal_Int32 nAbsPos = pOutliner->GetAbsPos( pPara ); + SAL_WARN_IF( nAbsPos > std::numeric_limits<sal_Int16>::max(), + "sd.core", + "SdPage::onParagraphRemoving: paragraph index " << nAbsPos + << " exceeds sal_Int16 range; ParagraphTarget.Paragraph will overflow" ); + aTarget.Paragraph = static_cast<sal_Int16>(nAbsPos); // Whether the paragraph before the removed one is empty, read from the // editing outliner that holds the live text. -- 2.54.0
