From 956c8f643219778c445d7062d1d0e7e3b96c7676 Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Wed, 21 Oct 2020 18:50:29 -0400 Subject: [PATCH] Obscure bug fix copying foreign streams in special cases (fixes #449) Specifically, if a stream had its stream data replaced and had indirect /Filter or /DecodeParms, it would result in non-silent loss of data and/or internal error. --- ChangeLog | 6 +++++ TODO | 1 - libqpdf/QPDF.cc | 26 +++++++++++++--------- make/msvc.mk | 4 ++-- qpdf/qtest/qpdf.test | 15 ++++++++++++- qpdf/qtest/qpdf/indirect-filter-out-0.pdf | Bin 0 -> 2671 bytes qpdf/qtest/qpdf/indirect-filter-out-1.pdf | Bin 0 -> 2671 bytes qpdf/qtest/qpdf/indirect-filter.pdf | Bin 0 -> 5188 bytes qpdf/test_driver.cc | 16 +++++++++++++ 9 files changed, 54 insertions(+), 14 deletions(-) create mode 100644 qpdf/qtest/qpdf/indirect-filter-out-0.pdf create mode 100644 qpdf/qtest/qpdf/indirect-filter-out-1.pdf create mode 100644 qpdf/qtest/qpdf/indirect-filter.pdf diff --git a/ChangeLog b/ChangeLog index 81812449..4a59b6a7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ 2020-10-21 Jay Berkenbilt + * Bug fix: properly handle copying foreign streams that have + indirect /Filter or /DecodeParms keys when stream data has been + replaced. The circumstances leading to this bug are very unusual + but would cause qpdf to either generate an internal error or some + other kind of warning situation if it would occur. Fixes #449. + * Qpdf's build and CI has been migrated from Azure Pipelines (Azure Devops) to GitHub Actions. diff --git a/TODO b/TODO index 32cbd901..3f4e0976 100644 --- a/TODO +++ b/TODO @@ -7,7 +7,6 @@ Candidates for upcoming release * Open "next" issues * bugs * #473: zsh completion with directories - * #449: internal error with case to reproduce (from pikepdf) * #444: concatenated stream/whitespace bug * Non-bugs * #446: recognize edited QDF files diff --git a/libqpdf/QPDF.cc b/libqpdf/QPDF.cc index 1cbef133..3b6d70ee 100644 --- a/libqpdf/QPDF.cc +++ b/libqpdf/QPDF.cc @@ -2313,7 +2313,8 @@ QPDF::reserveObjects(QPDFObjectHandle foreign, ObjCopier& obj_copier, if (foreign.isReserved()) { throw std::logic_error( - "QPDF: attempting to copy a foreign reserved object"); + "QPDF: attempting to copy a foreign reserved object: " + + QUtil::int_to_string(foreign.getObjectID())); } if (foreign.isPagesObject()) @@ -2486,6 +2487,16 @@ QPDF::replaceForeignIndirectObjects( } PointerHolder stream_buffer = stream->getStreamDataBuffer(); + // Note: at this stage, dictionary keys may still be reserved. + // We have to handle that explicitly if we access anything. + auto get_as_direct = [&old_dict] (std::string const& key) { + QPDFObjectHandle obj = old_dict.getKey(key); + obj.makeDirect(); + return obj; + }; + + QPDFObjectHandle filter = get_as_direct("/Filter"); + QPDFObjectHandle decode_parms = get_as_direct("/DecodeParms"); if ((foreign_stream_qpdf->m->immediate_copy_from) && (stream_buffer.getPointer() == 0)) { @@ -2495,8 +2506,7 @@ QPDF::replaceForeignIndirectObjects( // have to keep duplicating the memory. QTC::TC("qpdf", "QPDF immediate copy stream data"); foreign.replaceStreamData(foreign.getRawStreamData(), - dict.getKey("/Filter"), - dict.getKey("/DecodeParms")); + filter, decode_parms); stream_buffer = stream->getStreamDataBuffer(); } PointerHolder stream_provider = @@ -2504,9 +2514,7 @@ QPDF::replaceForeignIndirectObjects( if (stream_buffer.getPointer()) { QTC::TC("qpdf", "QPDF copy foreign stream with buffer"); - result.replaceStreamData(stream_buffer, - dict.getKey("/Filter"), - dict.getKey("/DecodeParms")); + result.replaceStreamData(stream_buffer, filter, decode_parms); } else if (stream_provider.getPointer()) { @@ -2515,8 +2523,7 @@ QPDF::replaceForeignIndirectObjects( this->m->copied_stream_data_provider->registerForeignStream( local_og, foreign); result.replaceStreamData(this->m->copied_streams, - dict.getKey("/Filter"), - dict.getKey("/DecodeParms")); + filter, decode_parms); } else { @@ -2534,8 +2541,7 @@ QPDF::replaceForeignIndirectObjects( this->m->copied_stream_data_provider->registerForeignStream( local_og, foreign_stream_data); result.replaceStreamData(this->m->copied_streams, - dict.getKey("/Filter"), - dict.getKey("/DecodeParms")); + filter, decode_parms); } } else diff --git a/make/msvc.mk b/make/msvc.mk index c8bc57bc..cfeaa45a 100644 --- a/make/msvc.mk +++ b/make/msvc.mk @@ -66,7 +66,7 @@ endef # Usage: $(call makelib,objs,library,ldflags,libs,current,revision,age) define makelib cl -nologo -O2 -Zi -Gy -EHsc -MD -LD -Fe$(basename $(2))$(shell expr $(5) - $(7)).dll $(1) \ - -link -SUBSYSTEM:CONSOLE,5.01 -incremental:no \ + -link -SUBSYSTEM:CONSOLE -incremental:no \ $(foreach L,$(subst -L,,$(3)),-LIBPATH:$(L)) \ $(foreach L,$(subst -l,,$(4)),$(L).lib) if [ -f $(basename $(2))$(shell expr $(5) - $(7)).dll.manifest ]; then \ @@ -81,7 +81,7 @@ endef define makebin cl -nologo -O2 -Zi -Gy -EHsc -MD $(1) \ $(if $(5),$(5),$(WINDOWS_MAIN_XLINK_FLAGS)) \ - -link -SUBSYSTEM:CONSOLE,5.01 -incremental:no -OUT:$(2) \ + -link -SUBSYSTEM:CONSOLE -incremental:no -OUT:$(2) \ $(foreach L,$(subst -L,,$(3)),-LIBPATH:$(L)) \ $(foreach L,$(subst -l,,$(4)),$(L).lib) if [ -f $(2).manifest ]; then \ diff --git a/qpdf/qtest/qpdf.test b/qpdf/qtest/qpdf.test index c2a38fa3..a0ff2a57 100644 --- a/qpdf/qtest/qpdf.test +++ b/qpdf/qtest/qpdf.test @@ -2417,7 +2417,7 @@ $td->runtest("check output", show_ntests(); # ---------- $td->notify("--- Copy Foreign Objects ---"); -$n_tests += 7; +$n_tests += 10; foreach my $d ([25, 1], [26, 2], [27, 3]) { @@ -2438,6 +2438,19 @@ $td->runtest("copy objects error", {$td->FILE => "copy-foreign-objects-errors.out", $td->EXIT_STATUS => 0}, $td->NORMALIZE_NEWLINES); + +$td->runtest("indirect filters", + {$td->COMMAND => "test_driver 69 indirect-filter.pdf"}, + {$td->STRING => "test 69 done\n", $td->EXIT_STATUS => 0}, + $td->NORMALIZE_NEWLINES); +foreach my $i (0, 1) +{ + $td->runtest("check output", + {$td->FILE => "auto-$i.pdf"}, + {$td->FILE => "indirect-filter-out-$i.pdf"}); +} + + show_ntests(); # ---------- $td->notify("--- Error Condition Tests ---"); diff --git a/qpdf/qtest/qpdf/indirect-filter-out-0.pdf b/qpdf/qtest/qpdf/indirect-filter-out-0.pdf new file mode 100644 index 0000000000000000000000000000000000000000..79e80601123b4db2083350fb8f9adec061b8ee2e GIT binary patch literal 2671 zcmb7Gc{G#@8-HgthA@aM)r&;RG8-oQV6tXklGHR_jF@3&xI{{~y+kHjK9m|%G9}VY z_Y2ph%}w^3Efv|5+{>-p=Dwp;U*8|!`JVTjIdh)p`906?_sp5|{`9?_7{){sGG4!C z?!rer5i|q2+oSQ;)}X03GZNy1Bsk*>n))ZlL7=H4Q^1VjMuN7sc!(1&ZIqT50gl`R zjsPTrrta);KDZeqFF>~}fLOBA6BjAISPwPCPa5j_9O@gPG!H+qS1X|EY zAUw;P$%7jgY578YZUT=5^ITxeg8xG&X1L8yqqCg5!3gQcR5Hl8^l(%+} zRPJvE(EZ35Jvm1kKMad3w|_9yWLI;jBj!}9*xm%#6)Ix|U+**I^Btvx7T3|)0=_rI zbL7UxaXC`4mhk1dF0uSgs4ij*Jq3RCCbO zFJZf2;kR2j#-iB8dzc2X!=)l<` z`iX)AjYUfXhs9#BvN$|m7AGr%PGjo$}S-+QBYJ;A}HbIl~t6LRN(JQ(jf>b z5{1UWKqWa@IXL@&hNKoC-~da2h(ZtmBmsdUASAy6S^$6oP*S>!fsw%hSXmSrE|Qf1 zq;xM5jmBUW>IftXjUfP79T{aJPFKajCtg)A{f!!l%xt0i`eoO_AWZ~-{EvYE8iPb( z0qHhl0szlLVvwK50f4j_O;E<@s1O}|-sqB4nel}!>E(6LrzPEhJPIxoPz1mZ7-QUU z*{3Mr^dxqPNE@9xHj3)q#3;KtyUDGktV!XXZF4vGz7BFk6(^i6DVrQK;2*tyvxQvm z(N>ZLOEbdCia zGV+;MXbn!aw?rWh3TKAH8ZN1Gb!R<|GiUd;jh@?D_4?g${N`h9ZrK^KR_UDIV+O*= z%ZSJCu227Xao9sQ>GV$JvuD+76Q9j_Zt+A`gx<#XTg+6M|Mtu5p=-w0!Reb^1JB%d zsb>}R&xW4Mde9y^;oNb3Lt_@XS~zFwJ5-Xp$>bDmT))7(!fUmhmGIcMxYA}o9#MHEV0kMv@?|R$_T9!X*g!p*g8QtiwfLs1s-b<8r9A#Q~I7X3rT72QT zf*#EvP3+dXql4NQZG6zFl4S65N&Slk4eRT{t1;z+dp42MoH8#x$q^eLdP;58$b0p7 zO;%CZ8Q;Fw)C8*52{o5adW%JqyrSMzenijcT0i3T3ZML*AeMe{#>*^*+Z%^^#ZxP7 zG%7T*FPOcEm&LUA)%mxU*#J}QT~|t02@C8zaT`-E2KNR%tJE=n7*!gb(LBXDXdtX?#G`-dG^5h)#N{>cy zW|hS$yYj2g}!a9NJ`Wt^ZlH6abXT~`V3WqTP+l~$NUjCm}}&kET=)h!L` zMC;3Fpw??Y?9{?AHDc6CclRbl!{Px5l7^rJ8aXdxw?k_$`92R&}TG_deytKXWXH_I|h> zC>w1!XkUB)SD$LQIXlhZl~v6#b-Bz)e$*Oppdt-o-E^x9Fj+TyYpS=T^QgOez9*F$ zOQ}paIo*sNX>nJ*q_K*z41%nc9Kav1sh`i{s5*45bhwjU^|$`8D8OH($4K8KHlPE$ zhRx6RrQvk4l;e9Hfb;|R5_8(wL-!0jk7tHNZHt-zAR73CHKti+5HR|zF$#*>dSk{# z^*S&;y_`pDWH9*0-aQewMIsw&1j!wvG8Vgz5G!nO3Efk{>}ytO&#IpL2U0Dp^2lOY z;7SqMWz(Kj&+quZe0Miwr1HV~$>9eKfgMYVGg)STgU`w=8#E zm&sPi)Bk$MC-I;NJZxt0W?up{r&7sOFarFNBf?HV8X)Iu&Kx!)-{puT*gk!iBU!@s z?YkTqc1>S$0v?kc0~;BsSMud@rCx2p-1)J0LLlrFT%Ewp)?^}uNTrd?VP2Lr3mTb3 owxpPo%qe8DC7EPFquT!OjxC=o5tAoakW8WwNqBvIXDNy{`|jF@3&xQdi+yF?~hKC}&rOhmfX z{fg_-!cF#@B?;LH_vTh^bKg;_ukVlVe9wE%oH@_){GR9cd*;k}fBN1|3}d1R8L$6f z?(#=G5i|q2J0tPd)}X03GaTZBBsk*(n)>aIfk0D7rhpm64F_#)@en6e+9)kA0vx$< z905oKP2Jg{d~gd$UVv_01b>?M6)G)%g>nQCN5BUupVoOop=>6dn*dKSgCBDu3ACV* zKzNonlLt30((-}$+&CT!=DEO_$7T6K0vKcp%K}YZqhSe-QdE%F&Pa$QSm<(%CV|w2 zHMX{}l5gpKBTPzUksX5_C4hLKDI z^b-XK8jF?&4vWQLWpQ}CEKXJyFRvtzms6CJl~o`pC@Lu{6O{4tDyk~Vs_=JZ=@5hz zi9+LGpt78-9Gv|>LsA70aDXL1L?H+Ol7K)F5RzX3EdW3PC@I~=z{ubLtSkx*7s<*1 zQo0w3Mq@Awbp#TH#t;Ckj*JQsr>p9)DOOD{<;_wOnb}15@y)D;L7E5v`5yxTGzN*n z0@7{91OT3g#2`P90|03=nxKNwQ6)NTdZSBHW5!-;N-3-Eo0fC}@+i1WKoI~tV3cvw z<)D&)(-q$#B5ibP+bF7W6Qk_r>?XDsvnGW5cg)>7Fd68GDu_E@STZqcz)vl`)kLoG z^7fEFopBAZk>_5zPEnC0QbYi3!QEr*3ii0BU&4R{u>N@F$3JJrDu&wwyQxe)ofG~? zjW*3IHU}lyTcQw0gtJ52YOktub>Dg#r@uYeGIC*i<>b4Q_$?>c+>&!-t>QV~rwoLV zml2QOS(Ebd%8-X{!r474=g+HG#rMv6ZuLZ#hup!wu$Zkh|LvEzM{gKg2c>Lw4LJA6 zrG}OF;%&%N%e_|1SqV?NO)3)b9_jTW7Y1QSF?tW#bxcEm>TO_ z6EvoUj=K}!Mc>mh@3~3w$$gKTfy;77j1V6$I-~3S6miRc%jrjH$55t)$D)*|XT_KM z6!mBZ$zr$WU2W95NaG_;6(oaxg_>8j8rG#jt1)E*`!|!4ozky9%Mu$OeNJuG$a($u zgN*!b=X`o5sc}@TQ%hai=}i_5@=AJBxnW%+Yki5O<(qQ50$KV6Y5f@tw>J(oN@rHu zXq0PYUN(CbD~oCEsrG9wu>q#ody5KJ3G?hcaT^n_1a$}YR_K^NiO3fp3JCLlDziVS zUjk6vi7bx4N~WjyY|n}wyMv>zn*#V<#xvPVIO8i5<1ajrsXLZ`TEjWxR#|Ya1fbuw zDNJ3K`9f5ze>}6^1FAKf_n3K@G8IGc{qWar>)1b6P_P$g9tlg9q%>Pzo0y{(b!il& zS6ZC0E4$toYn7ERAH4dN%W}LA;}pH20cq&%x=Q#k)5~D0xZEUc)MHt0M({4GZgF5c zT3<#3wO;#4yB3D25w)~hSE#6lcq-YCG=zf)(4;P(R$YX!=(U)h23WB0-cj zyyM>dZf#&6Q7x;=)&+6Dcl)K*M2|LSzWcQoGEmFZE4NKpiMd;E?9%g%A)G-2 zyUSbJn;Rkn3hvUYRtz}46wcZeq$(hGC#Gd-4cA1sPrfMoz1=c#P5eB0TbK~lsdn_) zjyzt2jCQ&$HvMAy8i(=h$QInS+~jbmIn5@hZT;o}W8=By zo#Mp$zzu#AswXgwx92{_4VLwIa0r_2wMJVS%FWo;r-m+wuRL7!3OTEIbseFDqm|=V z@yv^oMZT_Fs}?omuD3E$*Ra0K8iNuRYs#e^7*ek0HwikL)t$!PZz?PJnPWM4;KQ8& z*+|0y`+~!`nk2(5naKvPtsb0EmrD=lN2~!~mM253TW;0c6~ML-eTZ_c`? zl>*b#%Xzdq27`a%-7|4ZII{MEV0YVyjK$vL#Bv*4T<25}`-W9=Z)Klf0M)`Ohb)!_ zit@=WoAnRCzbo!|Lo^5ruR z^x)b+wltFY)#;OONDxQ?MKOsaM@P^ukQWb2KpNr+0_{RJr@)|{J5S0>62*hg&LmhE ziw08XB)E%Vo>U+bdhnz$Xz8(Wje3i4t?RFGv)1(8~TJTVeDJ8cjw5v7Uw$oR^G5R3T1uoOwMLm-28-pRGdGS6IibH+o#zlV;kO#2_BLq`2ubm%e7qT>pZkkx>8 z+$3b39xz`Ni(CLdSQsx&0O=4ql0=FuJeibHymF9Q6G8zv!8C%Iy1P>_flpK4F&J&T zwJqA6a44(V)fU(qt?GbWncq`>zdR_Q*>)U(R1ye_-9^bMA|bj8HgerWNg{D@3XhL; z_JGp`d^l(g2SM}}SriDNOAG>0LzdwM3*r-`AcG0o1*gSGl^0gIs@YY{Zp$uQ5Q`3B z|HnKKn=yAH#Se=6fVL0Un+sqt0N{nZ07W0*4&bn;q3<}v@G5xJh$<=s6*VG>q()Rz zBWY-9kkmER)zlV{7iel}Ym>D}8alc<+PcVRZL|mm&BWn}NTRm7nmXeB4^7bkkcj{r zfN&TxfF)yaWQ^ilzz6_v01kyaYXnsyprVGuBM)iX02b|w#p4MC<#P-chbNE$6%$n* zh-j+owk}oAEa%xGDvj692?{P~M3M|K0QN5h0XzYVQvuL!He>*)hb3TtF9!fhV%fwCB_`KA2S80W@&PV=|5mxB#QvFT6h0k_!9Md*#%%9^Gr@O+M8) zmuZ*rjg|ay*|ts77q&kQcgK~d9j&MuAGMI|JAa{_))Wxvr}0_-Y0O%&@A*}lnpC+a z1`tRu?Ge-oo*0H?3@QM}H-|p?<@s3MNKbe_gJ)*4KXkYCx*5%mh%8q&4zo)(HN2tu zl)lLHg}+VSi%&ZrAKO^}^yL9k)P8}e>IluKays}H7h@e@EtcGA%6aqou%Bth;Vn8x zkLow1-<$UTz#m&1eNp8uYpS04{WmXmpR;j{$XV|lcH}#+CVuJN7tzP^uXja1@$5do zrX`F$x3}}l4MQg`MqWM_HmP2_>}K2Z`;*@dhu$h({ESv%QhSd8hFY`V;CJ= zv;sS4}Zp(V40O-CDUl^)W;9q@s zVCjI%r=vf$he`Tuo);|?K3SHTe(b7h%bv2&3_SBM)I^pj0L~?+ihYXCI_jQha)fY=k z+qkGeD&Owv>ipp6!Ld>yGrGO2N;v-UnN#uvUKVe&+1QC6*G3D6EL=`T^>nl*hLvC9 zG%Ou-zb~6|Dc`pMvpF-j&}gJ7x##KKnjd=DnO5mDv<-1G+#S8$w>OoFTUCwooK^CU z=UKTuDN5`lo-WCbhdXkeBDz<Cfd5Ir;Rhq z+>kF`)xO;~DI(Hig`-$|4%8DfBR8R zVdvo<@>ZFcm7_Ama2$yOLJ*1ahk;B6gT?^kz(0HtqQamC3jg#m5#8t=A4GpYj*2#a zCyvHMNX|Px1{Kj#-tjSMh~V*8AC<{?-^Y5t9-YOW(?cre36cfQ8B2$WV zK_Zb9L?t{#unQJ!fsu1S0zAMdM>?Gs!-(NgAwDmL#ia6KHj~9