Rework inline image parsing in content streams Currently, CPDF_StreamContentParser:Handle_BeginImage() will try to parse inline images in content streams when it sees the "BI" operator. The parts that deal with the related "ID" and "EI" operators (Handle_BeginImageData() and Handle_EndImage(), respectively) are no-ops, since Handle_BeginImage() did all the work. Rework Handle_BeginImage() and Handle_BeginImageData() so CPDF_StreamContentParser records their respective keywords's positions. Then handle the inline image parsing in Handle_EndImage() with those positions. At this point, CPDF_StreamContentParser know the distance between the "ID" and "EI" operators, so it knows where the length of the inline image stream. Pass this length to CPDF_StreamParser::ReadInlineStream() and prevent it from reading the "EI" operator or past it. As a result of this change: - Handle_BeginImage() no longer needs to look for "ID" and "EI". - ReadInlineStream() no longer needs to looks for "EI". - bug_1236805.in fails because the code was reading "EI" and past it. Fix this by adding a placeholder value so the inline stream is not empty. Remove the temporary suppression for this file. - The bug_407752631.in test case can be checked in and will no longer cause a crash. To deal with malforms images that may not have "BI", "ID", and "EI" operators in that order, do some state tracking so that CPDF_StreamContentParser only looks for "ID" and "EI" when appropriate, like the existing code. Bug: 407752631 Change-Id: Ie14908d8ed72c92c2ae880c18f62469a5cf4eef3 Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/130911 Commit-Queue: Lei Zhang <thestig@chromium.org> Reviewed-by: Tom Sepez <tsepez@chromium.org>
diff --git a/core/fpdfapi/page/cpdf_streamcontentparser.cpp b/core/fpdfapi/page/cpdf_streamcontentparser.cpp index fdd3ac9..8e12bef 100644 --- a/core/fpdfapi/page/cpdf_streamcontentparser.cpp +++ b/core/fpdfapi/page/cpdf_streamcontentparser.cpp
@@ -10,6 +10,7 @@ #include <array> #include <map> #include <memory> +#include <optional> #include <utility> #include <vector> @@ -41,11 +42,13 @@ #include "core/fxcrt/check.h" #include "core/fxcrt/compiler_specific.h" #include "core/fxcrt/containers/contains.h" +#include "core/fxcrt/fx_memory.h" #include "core/fxcrt/fx_safe_types.h" #include "core/fxcrt/notreached.h" #include "core/fxcrt/scoped_set_insertion.h" #include "core/fxcrt/span.h" #include "core/fxcrt/stl_util.h" +#include "core/fxcrt/unowned_ptr.h" #include "core/fxge/cfx_graphstate.h" #include "core/fxge/cfx_graphstatedata.h" @@ -257,6 +260,20 @@ } } +template <typename T> +class AutoOptionalResetter { + public: + FX_STACK_ALLOCATED(); + + explicit AutoOptionalResetter(std::optional<T>* value) : value_(value) { + CHECK(value_); + } + ~AutoOptionalResetter() { value_->reset(); } + + private: + const UnownedPtr<std::optional<T>> value_; +}; + } // namespace // static @@ -589,7 +606,25 @@ } void CPDF_StreamContentParser::OnOperator(ByteStringView op) { - auto it = g_opcodes->find(op.GetID()); + const uint32_t op_id = op.GetID(); + if (inline_image_context_.has_value()) { + switch (inline_image_context_.value().state) { + case InlineImageContext::State::kLookingForID: + // Also tolerate the case with "ID" missing. + if (op_id != FXBSTR_ID('I', 'D', 0, 0) && + op_id != FXBSTR_ID('E', 'I', 0, 0)) { + return; + } + break; + case InlineImageContext::State::kLookingForEI: + if (op_id != FXBSTR_ID('E', 'I', 0, 0)) { + return; + } + break; + } + } + + auto it = g_opcodes->find(op_id); if (it != g_opcodes->end()) { (this->*it->second)(); } @@ -640,63 +675,8 @@ } void CPDF_StreamContentParser::Handle_BeginImage() { - FX_FILESIZE savePos = syntax_->GetPos(); - auto pDict = document_->New<CPDF_Dictionary>(); - while (true) { - CPDF_StreamParser::ElementType type = syntax_->ParseNextElement(); - if (type == CPDF_StreamParser::ElementType::kKeyword) { - if (syntax_->GetWord() != "ID") { - syntax_->SetPos(savePos); - return; - } - } - if (type != CPDF_StreamParser::ElementType::kName) { - break; - } - // Next `syntax_` read below may invalidate `word`. Must save to `key`. - ByteStringView word = syntax_->GetWord(); - ByteString key(word.Last(word.GetLength() - 1)); - auto pObj = syntax_->ReadNextObject(false, false, 0); - if (pObj && !pObj->IsInline()) { - pDict->SetNewFor<CPDF_Reference>(key, document_, pObj->GetObjNum()); - } else { - pDict->SetFor(key, std::move(pObj)); - } - } - ReplaceAbbr(pDict); - RetainPtr<const CPDF_Object> pCSObj; - if (pDict->KeyExist("ColorSpace")) { - pCSObj = pDict->GetDirectObjectFor("ColorSpace"); - if (pCSObj->IsName()) { - ByteString name = pCSObj->GetString(); - if (name != "DeviceRGB" && name != "DeviceGray" && name != "DeviceCMYK") { - pCSObj = FindResourceObj("ColorSpace", name); - if (pCSObj && pCSObj->IsInline()) { - pDict->SetFor("ColorSpace", pCSObj->Clone()); - } - } - } - } - pDict->SetNewFor<CPDF_Name>("Subtype", "Image"); - RetainPtr<CPDF_Stream> pStream = - syntax_->ReadInlineStream(document_, std::move(pDict), pCSObj.Get()); - while (true) { - CPDF_StreamParser::ElementType type = syntax_->ParseNextElement(); - if (type == CPDF_StreamParser::ElementType::kEndOfData) { - return; - } - - if (type == CPDF_StreamParser::ElementType::kKeyword && - syntax_->GetWord() == "EI") { - break; - } - } - CPDF_ImageObject* pObj = AddImageFromStream(std::move(pStream), /*name=*/""); - // Record the bounding box of this image, so rendering code can draw it - // properly. - if (pObj && pObj->GetImage()->IsMask()) { - object_holder_->AddImageMaskBoundingBox(pObj->GetRect()); - } + CHECK(!inline_image_context_.has_value()); + inline_image_context_.emplace().begin_pos = syntax_->GetPos(); } void CPDF_StreamContentParser::Handle_BeginMarkedContent() { @@ -899,7 +879,79 @@ void CPDF_StreamContentParser::Handle_MarkPlace_Dictionary() {} -void CPDF_StreamContentParser::Handle_EndImage() {} +void CPDF_StreamContentParser::Handle_EndImage() { + if (!inline_image_context_.has_value()) { + return; // No matching "BI" operator. + } + + AutoOptionalResetter auto_resetter(&inline_image_context_); + const InlineImageContext& inline_image_context = + inline_image_context_.value(); + if (inline_image_context.state != InlineImageContext::State::kLookingForEI) { + return; // No matching "ID" operator. + } + + CHECK_LT(inline_image_context.begin_pos, inline_image_context.data_pos); + + const uint32_t saved_position = syntax_->GetPos(); + auto synthesized_dict = document_->New<CPDF_Dictionary>(); + syntax_->SetPos(inline_image_context.begin_pos); + while (true) { + CPDF_StreamParser::ElementType type = syntax_->ParseNextElement(); + if (syntax_->GetPos() >= inline_image_context.data_pos) { + break; // Reached "ID" operator. + } + if (type == CPDF_StreamParser::ElementType::kKeyword) { + continue; // Already handled by CPDF_StreamContentParser itself. + } + if (type != CPDF_StreamParser::ElementType::kName) { + break; + } + // Next `syntax_` read below may invalidate `word`. Must save to `key`. + ByteStringView word = syntax_->GetWord(); + ByteString key(word.Last(word.GetLength() - 1)); + auto value_object = syntax_->ReadNextObject(false, false, 0); + if (value_object && !value_object->IsInline()) { + synthesized_dict->SetNewFor<CPDF_Reference>(key, document_, + value_object->GetObjNum()); + } else { + synthesized_dict->SetFor(key, std::move(value_object)); + } + } + ReplaceAbbr(synthesized_dict); + synthesized_dict->SetNewFor<CPDF_Name>("Subtype", "Image"); + + RetainPtr<const CPDF_Object> colorspace; + if (synthesized_dict->KeyExist("ColorSpace")) { + colorspace = synthesized_dict->GetDirectObjectFor("ColorSpace"); + if (colorspace->IsName()) { + ByteString name = colorspace->GetString(); + if (name != "DeviceRGB" && name != "DeviceGray" && name != "DeviceCMYK") { + colorspace = FindResourceObj("ColorSpace", name); + if (colorspace && colorspace->IsInline()) { + synthesized_dict->SetFor("ColorSpace", colorspace->Clone()); + } + } + } + } + + syntax_->SetPos(inline_image_context.data_pos); + FX_SAFE_UINT32 stream_length = saved_position; + stream_length -= inline_image_context.data_pos; + stream_length -= 2; // Account for "EI" operator. + RetainPtr<CPDF_Stream> inline_stream = + syntax_->ReadInlineStream(document_, std::move(synthesized_dict), + colorspace, stream_length.ValueOrDie()); + CPDF_ImageObject* new_image_object = + AddImageFromStream(std::move(inline_stream), /*name=*/""); + // Record the bounding box of this image, so rendering code can draw it + // properly. + if (new_image_object && new_image_object->GetImage()->IsMask()) { + object_holder_->AddImageMaskBoundingBox(new_image_object->GetRect()); + } + + syntax_->SetPos(saved_position); +} void CPDF_StreamContentParser::Handle_EndMarkedContent() { // First element is a sentinel, so do not pop it, ever. This may come up if @@ -975,7 +1027,20 @@ cur_states_->mutable_general_state().SetFlatness(GetNumber(0)); } -void CPDF_StreamContentParser::Handle_BeginImageData() {} +void CPDF_StreamContentParser::Handle_BeginImageData() { + InlineImageContext& inline_image_context = inline_image_context_.value(); + CHECK_EQ(inline_image_context.state, + InlineImageContext::State::kLookingForID); + CHECK_EQ(inline_image_context.data_pos, 0u); + + inline_image_context.state = InlineImageContext::State::kLookingForEI; + + uint32_t pos = syntax_->GetPos(); + // Something else, like the "BI" operator, must be before the "ID" operator, + // so "ID" cannot be at position 0. + CHECK_NE(pos, 0u); + inline_image_context.data_pos = pos; +} void CPDF_StreamContentParser::Handle_SetLineJoin() { cur_states_->mutable_graph_state().SetLineJoin(
diff --git a/core/fpdfapi/page/cpdf_streamcontentparser.h b/core/fpdfapi/page/cpdf_streamcontentparser.h index 80b097c..a1bc9ed 100644 --- a/core/fpdfapi/page/cpdf_streamcontentparser.h +++ b/core/fpdfapi/page/cpdf_streamcontentparser.h
@@ -9,6 +9,7 @@ #include <array> #include <memory> +#include <optional> #include <stack> #include <variant> #include <vector> @@ -73,6 +74,14 @@ private: enum class RenderType : bool { kFill = false, kStroke = true }; + struct InlineImageContext { + enum class State : bool { kLookingForID, kLookingForEI }; + + State state = State::kLookingForID; + uint32_t begin_pos = 0; + uint32_t data_pos = 0; + }; + using ContentParam = std::variant<RetainPtr<CPDF_Object>, FX_Number, ByteString>; @@ -242,6 +251,8 @@ // The merged stream offset at which the last |syntax_| started parsing. uint32_t start_parse_offset_ = 0; + + std::optional<InlineImageContext> inline_image_context_; }; #endif // CORE_FPDFAPI_PAGE_CPDF_STREAMCONTENTPARSER_H_
diff --git a/core/fpdfapi/page/cpdf_streamparser.cpp b/core/fpdfapi/page/cpdf_streamparser.cpp index 4bdfb61..7f750ff 100644 --- a/core/fpdfapi/page/cpdf_streamparser.cpp +++ b/core/fpdfapi/page/cpdf_streamparser.cpp
@@ -25,7 +25,6 @@ #include "core/fxcodec/data_and_bytes_consumed.h" #include "core/fxcodec/jpeg/jpegmodule.h" #include "core/fxcodec/scanlinedecoder.h" -#include "core/fxcrt/autorestorer.h" #include "core/fxcrt/check.h" #include "core/fxcrt/data_vector.h" #include "core/fxcrt/fx_extension.h" @@ -140,14 +139,14 @@ RetainPtr<CPDF_Stream> CPDF_StreamParser::ReadInlineStream( CPDF_Document* pDoc, RetainPtr<CPDF_Dictionary> pDict, - const CPDF_Object* pCSObj) { - auto stream_span = buf_.subspan(pos_); + const CPDF_Object* pCSObj, + uint32_t stream_length) { + auto stream_span = buf_.subspan(pos_).first(stream_length); if (stream_span.empty()) { return nullptr; } if (PDFCharIsWhitespace(stream_span.front())) { - pos_++; stream_span = stream_span.subspan<1>(); if (stream_span.empty()) { return nullptr; @@ -201,7 +200,6 @@ auto src_span = stream_span.first(original_size); data = DataVector<uint8_t>(src_span.begin(), src_span.end()); actual_stream_size = original_size; - pos_ += original_size; } else { actual_stream_size = DecodeInlineStream(stream_span, width, height, decoder, @@ -210,26 +208,8 @@ return nullptr; } - { - AutoRestorer<uint32_t> saved_position(&pos_); - pos_ += actual_stream_size; - while (true) { - uint32_t saved_iteration_position = pos_; - ElementType type = ParseNextElement(); - if (type == ElementType::kEndOfData) { - return nullptr; - } - - if (type == ElementType::kKeyword && GetWord() == "EI") { - break; - } - - actual_stream_size += pos_ - saved_iteration_position; - } - } auto src_span = stream_span.first(actual_stream_size); data = DataVector<uint8_t>(src_span.begin(), src_span.end()); - pos_ += actual_stream_size; } pDict->SetNewFor<CPDF_Number>("Length", static_cast<int>(actual_stream_size)); return pdfium::MakeRetain<CPDF_Stream>(std::move(data), std::move(pDict));
diff --git a/core/fpdfapi/page/cpdf_streamparser.h b/core/fpdfapi/page/cpdf_streamparser.h index 417f5f2..a00cc71 100644 --- a/core/fpdfapi/page/cpdf_streamparser.h +++ b/core/fpdfapi/page/cpdf_streamparser.h
@@ -44,7 +44,8 @@ uint32_t dwRecursionLevel); RetainPtr<CPDF_Stream> ReadInlineStream(CPDF_Document* pDoc, RetainPtr<CPDF_Dictionary> pDict, - const CPDF_Object* pCSObj); + const CPDF_Object* pCSObj, + uint32_t stream_length); private: friend class CPDFStreamParserTest_ReadHexString_Test;
diff --git a/testing/SUPPRESSIONS b/testing/SUPPRESSIONS index ae5308a..aa1db92 100644 --- a/testing/SUPPRESSIONS +++ b/testing/SUPPRESSIONS
@@ -622,9 +622,6 @@ # TODO(chromium:1131694): Remove after associated bug is fixed bug_1131694.in * * * * -# TODO(thestig): Remove after fixing inline image parsing. -bug_1236805.in * * * * - # TODO(pdfium:1747): Remove after associated bug is fixed bug_1258634.in * * * *
diff --git a/testing/resources/pixel/bug_1236805_expected.pdf.0.png b/testing/resources/pixel/bug_1236805_expected.pdf.0.png index 7d4eebe..08c11b0 100644 --- a/testing/resources/pixel/bug_1236805_expected.pdf.0.png +++ b/testing/resources/pixel/bug_1236805_expected.pdf.0.png Binary files differ
diff --git a/testing/resources/pixel/bug_407752631.in b/testing/resources/pixel/bug_407752631.in new file mode 100644 index 0000000..3993fa4 --- /dev/null +++ b/testing/resources/pixel/bug_407752631.in
@@ -0,0 +1,69 @@ +{{header}} +{{object 1 0}} << + /Type /Catalog + /Pages 2 0 R +>> +endobj +{{object 2 0}} << + /Type /Pages + /Count 1 + /Kids [3 0 R] + /MediaBox [0 0 200 200] +>> +endobj +{{object 3 0}} << + /Type /Page + /Parent 2 0 R + /Contents 4 0 R +>> +endobj +{{object 4 0}} << + /Filter [ + % The stream contains a few hundred megabytes of spaces. Do a few rounds of + % compression to reduce the file size. + /ASCII85Decode + /FlateDecode + /ASCII85Decode + /FlateDecode + /ASCII85Decode + /FlateDecode + ] + {{streamlen}} +>> +stream +GhT!\fp\An(5C/G$NX+E5XeK\'GUb@;+(Ei6pVM%E&d6#hpohD-_7E?O=04LO#=ZO>,%oD/DX*[+M+AH +n@\iELsngCp8V,_>&67C1Ck_\NC('?^J9rQ:5YAbfgouJk$tOnR.c1L:(6k[0Bc\"VOmu:([Q2:\]e>S +2o35";9H3iiCU_#G+I/NM"9J[3=PIUaJBQ*K7`]DX*EH,ADNiI$\W\)*%@N":8=s+]kU1PR-8&p\k-GP +2.uFEWbfLNl!ef$?B^R'"nQ.,Ou%%M=a:<ZMs^X.DjfWFV(X@5*JNTeS_FAA-Kj6`l11DGB-u!$>YTa5 +-a$sNpJ?ol^EKZtT''G8.5PB%lMWUP\B4/9X[1.Fm7cNaL^0o!^6,71c8sniWe'7"SRug4A3Jk/c,O+P +]!>JJ;R_Y0i&!UCN\o(u$)']kWu*U!9TmG8*!,f=G=W^tCPZD%q68T"F>3:r;bA&^F$&j&ok/lHI$.QX +3Ws@K*[p.!TiF\0[u72J!6JC2(c?d=o>b$mp3C]7O82IGcL/5t3n+5Hg=YH>.'3+iT?$PUC)q+U)h1sp +SlDSUbZrHPEVQ>PV4)a17_gZ@riTU!R*"L^L;&Dg>4]-CN=p25XCK4ic^>lOk?&nAHjdk`2F27f"ZA1J +=f$][QS)armMr;\C[_'"]W8GDk+FDb;s2GRbG68Q_'nRInt<nBXVF&"q`c,;:8MHVGa8!F;oH,l0#FN; +L0JhZ8B2%rX2u&)"*Y^%-i7-%/e"]K*kZ(s#[9bPfS`\"^J2)d%C,\"0<FhH"\f)$2`#roQlSW#bJQ+0 +B>2pWFU%2?">=q`Emh4dd2+keN[#!VdPSpGY*i1gl=N6jO];[DH7c+[^ACb;InGef-%%Ht%92cFWPeT7 +_PP_Xd:.d:Z-S'tYn%(3k3U]T]?ZR9ZQ!'3c#%J#O;G/,B.udsJ3_GcE='1$V23)ViHcQJ%u=I0E,cT3 +8=kA^+hX?SZmCXf%k@u8?RmdD/g[J&bZ\d<f$#85@+uNp8?.h&;,ZOWh=6u2T"43*`!i#o;o'b$<8L<G +$-(T1]I"\t6h[f9W\&=d')^G*lGe^oU9Fd=6UK(QQ'qK)]I2.A**\8d#L6T1>O0_[dM'(\>laX^7KP42 +/!@WgNKNjLAQKC[c,pm0AMO5Oer.;CnorV_1H2#m%6'a+EUJ))NDJA^aI8Z;j@li)9*Y<pAq_I3m6air +eZ#9G]C;=iRLNqI2Ri?ngjo[FIKF[52!m$4:R:aj!rN(%OS]aYAC]);?&,$t&%RUM_Z5\3H9L<2:gHoj +=,X/G5q9g82B743=unGpqsuW?Z/@6?//K572Nr+7A@aYG;YC!NVa#!Xs3Sb.NMaA-RI^[tWnFG>P>WKc +jZPhY59\42>aPh%I_jhI5fOB5j=4YqBIM^@4mik2^=8qA`q6Y/i;iul4m=C_U+s+X7)MZqG`M8!pd1$^ +V/7.#Ss#o0*fa"4LtGqqY9rAO8"K=];#*ETQRl&?LO6MM4#q]'=076j7P;4RX6X)?^m<A?CL.KZX$TDn +I/:s\r"PQt8_c4Ef-d7p]8U>+a#?/paf#tV"T<-n!On]AoW;tV"XBKtaD.T5HEPpZ)0CiU,#St+Eq*?q +*KWCTI9C#nVof.]W0XT$KlT/!GaI=B_dmOd,U@N@"5%[,oUt/[S(.+R*fR3T:2%/aej/lo2a%UE^$MDB +GL%nG2qgUb2nit>\NPNta$c"eCDb1KXn8c)p`.!RL:'gCV&r\R-r_X;!]D%'phh>!KJa>p(`lFYXn]gm +NTHm3UfVBH`IR!neB_DWa/!W.,5ES\]N8ff#0h*LBm#+e'nCB2[[48.>5Ai0I)=Q!!I1g0XQ-7t<d8:_ +BPBPWb/A.ZKp'UC&LLV'KuSrl6TO!?0XU@QNkK\N77-g+f%Z\Mn_V+\M^^ROmYd#7fTeI!RWWu5&6dj3 +,F-AP8ZH3\r8H$(]Q\L[ak?LP@pYYiWYh`)1[u/u_p0h4,t4b`Ep@9mf:HGEU(W^BrLkU)nY/\-2uB>< +1o/d!pSKgYoic'Ni%916M[troWAYl%59R;2nK+>ei=cP]JUggfdmY<hSE/Y;:="XSmYla`BZ)Ec(?S0N +EAaAX%\LLS$O7eqlWdfa.&W&G_'E'IrkGSBn"E(WPR@u8<QGL;4hj0,9*8S[AdB[ekIt@WG=g;-NBZ$% +M)[N4cP[$a@#i?>N('9(lRNUZ<qWh[5b(t\"=mP777gQ_8$_1$ZKgi)@S;($/Pa&H*\7q^TW`M#R^Gp< +J,M!Le'^s$1nVA*Ug\q/r)Vs7*a>k(no*>MJC\`Rf#1Mc/ke;hUN4o]jZg;.J=d=F5JDc#Gg-3!X].;] +hX,&5T&Ga2b:)8hRH83=Ri^/B0TE21pL)t\=g7C?Xe3gO>/$WP9U"o@0>X6Fg4(u1rrJ1r11^~> +endstream +endobj +{{xref}} +{{trailer}} +{{startxref}} +%%EOF
diff --git a/testing/resources/pixel/bug_407752631_expected.pdf.0.png b/testing/resources/pixel/bug_407752631_expected.pdf.0.png new file mode 100644 index 0000000..f97e340 --- /dev/null +++ b/testing/resources/pixel/bug_407752631_expected.pdf.0.png Binary files differ
