From 192497124e7cde747ade7bf89028586eea293be5 Mon Sep 17 00:00:00 2001 From: tsepez Date: Thu, 12 Jan 2017 11:15:04 -0800 Subject: Custom toString() methods may delete annots. In this case, we observe the destruction of the object, but have unfortunately saved a pointer to it in a local variable. BUG=679643 Review-Url: https://codereview.chromium.org/2628233002 --- fpdfsdk/javascript/Annot.cpp | 49 +++++--- fpdfsdk/javascript/JS_Value.h | 5 +- testing/resources/javascript/bug_679643.in | 135 +++++++++++++++++++++ .../resources/javascript/bug_679643_expected.txt | 3 + 4 files changed, 171 insertions(+), 21 deletions(-) create mode 100644 testing/resources/javascript/bug_679643.in create mode 100644 testing/resources/javascript/bug_679643_expected.txt diff --git a/fpdfsdk/javascript/Annot.cpp b/fpdfsdk/javascript/Annot.cpp index 0c16b3b05a..1aef4634f2 100644 --- a/fpdfsdk/javascript/Annot.cpp +++ b/fpdfsdk/javascript/Annot.cpp @@ -38,20 +38,24 @@ Annot::Annot(CJS_Object* pJSObject) : CJS_EmbedObj(pJSObject) {} Annot::~Annot() {} bool Annot::hidden(IJS_Context* cc, CJS_PropValue& vp, CFX_WideString& sError) { - CPDFSDK_BAAnnot* baAnnot = ToBAAnnot(m_pAnnot.Get()); - if (!baAnnot) - return false; - if (vp.IsGetting()) { - CPDF_Annot* pPDFAnnot = baAnnot->GetPDFAnnot(); + if (!m_pAnnot) { + sError = JSGetStringFromID(IDS_STRING_JSBADOBJECT); + return false; + } + CPDF_Annot* pPDFAnnot = ToBAAnnot(m_pAnnot.Get())->GetPDFAnnot(); vp << CPDF_Annot::IsAnnotationHidden(pPDFAnnot->GetAnnotDict()); return true; } bool bHidden; - vp >> bHidden; + vp >> bHidden; // May invalidate m_pAnnot. + if (!m_pAnnot) { + sError = JSGetStringFromID(IDS_STRING_JSBADOBJECT); + return false; + } - uint32_t flags = baAnnot->GetFlags(); + uint32_t flags = ToBAAnnot(m_pAnnot.Get())->GetFlags(); if (bHidden) { flags |= ANNOTFLAG_HIDDEN; flags |= ANNOTFLAG_INVISIBLE; @@ -63,23 +67,28 @@ bool Annot::hidden(IJS_Context* cc, CJS_PropValue& vp, CFX_WideString& sError) { flags &= ~ANNOTFLAG_NOVIEW; flags |= ANNOTFLAG_PRINT; } - baAnnot->SetFlags(flags); + ToBAAnnot(m_pAnnot.Get())->SetFlags(flags); return true; } bool Annot::name(IJS_Context* cc, CJS_PropValue& vp, CFX_WideString& sError) { - CPDFSDK_BAAnnot* baAnnot = ToBAAnnot(m_pAnnot.Get()); - if (!baAnnot) - return false; - if (vp.IsGetting()) { - vp << baAnnot->GetAnnotName(); + if (!m_pAnnot) { + sError = JSGetStringFromID(IDS_STRING_JSBADOBJECT); + return false; + } + vp << ToBAAnnot(m_pAnnot.Get())->GetAnnotName(); return true; } CFX_WideString annotName; - vp >> annotName; - baAnnot->SetAnnotName(annotName); + vp >> annotName; // May invalidate m_pAnnot. + if (!m_pAnnot) { + sError = JSGetStringFromID(IDS_STRING_JSBADOBJECT); + return false; + } + + ToBAAnnot(m_pAnnot.Get())->SetAnnotName(annotName); return true; } @@ -88,12 +97,12 @@ bool Annot::type(IJS_Context* cc, CJS_PropValue& vp, CFX_WideString& sError) { sError = JSGetStringFromID(IDS_STRING_JSREADONLY); return false; } - - CPDFSDK_BAAnnot* baAnnot = ToBAAnnot(m_pAnnot.Get()); - if (!baAnnot) + if (!m_pAnnot) { + sError = JSGetStringFromID(IDS_STRING_JSBADOBJECT); return false; - - vp << CPDF_Annot::AnnotSubtypeToString(baAnnot->GetAnnotSubtype()); + } + vp << CPDF_Annot::AnnotSubtypeToString( + ToBAAnnot(m_pAnnot.Get())->GetAnnotSubtype()); return true; } diff --git a/fpdfsdk/javascript/JS_Value.h b/fpdfsdk/javascript/JS_Value.h index ff2c620980..313f0c3e7b 100644 --- a/fpdfsdk/javascript/JS_Value.h +++ b/fpdfsdk/javascript/JS_Value.h @@ -92,6 +92,7 @@ class CJS_PropValue { CJS_Runtime* GetJSRuntime() const { return m_pJSRuntime; } CJS_Value* GetJSValue() { return &m_Value; } + // These calls may re-enter JS (and hence invalidate objects). void operator<<(int val); void operator>>(int&) const; void operator<<(bool val); @@ -127,13 +128,15 @@ class CJS_Array { virtual ~CJS_Array(); void Attach(v8::Local pArray); + int GetLength(CJS_Runtime* pRuntime) const; + + // These two calls may re-enter JS (and hence invalidate objects). void GetElement(CJS_Runtime* pRuntime, unsigned index, CJS_Value& value) const; void SetElement(CJS_Runtime* pRuntime, unsigned index, const CJS_Value& value); - int GetLength(CJS_Runtime* pRuntime) const; v8::Local ToV8Array(CJS_Runtime* pRuntime) const; diff --git a/testing/resources/javascript/bug_679643.in b/testing/resources/javascript/bug_679643.in new file mode 100644 index 0000000000..e9643860f7 --- /dev/null +++ b/testing/resources/javascript/bug_679643.in @@ -0,0 +1,135 @@ +{{header}} +{{object 1 0}} << + /Type /Catalog + /Pages 2 0 R + /AcroForm 4 0 R + /OpenAction 10 0 R +>> +endobj +{{object 2 0}} << + /Type /Pages + /Count 1 + /Kids [ + 3 0 R + ] +>> +endobj +% Page number 0. +{{object 3 0}} << + /Type /Page + /Parent 2 0 R + /Resources << + /Font <> + >> + /Contents [21 0 R] + /MediaBox [0 0 612 792] + /Annots [7 0 R 8 0 R 9 0 R] +>> +endobj +% Forms +{{object 4 0}} << + /XFA [ + (xdp:xdp) 23 0 R + (form) 29 0 R + () 30 0 R + ] + /Fields [ + 5 0 R + ] +>> +endobj +% Fields +{{object 5 0}} << + /T (MyField) + /Kids [ + 6 0 R + ] + /Rect [100 100 400 400] +>> +endobj +{{object 6 0}} << + /Parent 5 0 R + /FT /Btn + /Kids [ + 7 0 R + 8 0 R + 9 0 R + ] + /Rect [200 200 220 220] +>> +endobj +{{object 7 0}} << + /Parent 6 0 R + /Type /Annot + /Subtype /Widget + /Rect [220 220 240 240] +>> +endobj +{{object 8 0}} << + /Parent 6 0 R + /Type /Annot + /Subtype /Widget + /Rect [240 240 260 260] +>> +endobj +{{object 9 0}} << + /Parent 6 0 R + /Type /Annot + /Subtype /Widget + /Rect [240 240 260 260] +>> +endobj +% OpenAction action +{{object 10 0}} << + /Type /Action + /S /JavaScript + /JS 11 0 R +>> +endobj +% JS program to exexute +{{object 11 0}} << +>> +stream +var theName = "MyField"; +function Mangles() { + app.alert('Starting ...'); + try { + var annots = this.getAnnots(); + annots[0].name = { + toString: () => { + app.alert('Firing ...'); + this.removeField(theName); + gc(); + return false; + } + }; + } catch (e) { + app.alert("failed: " + e); + } +} +Mangles(); +endstream +endobj +{{object 23 0}} << +>>stream + + +endstream +endobj +{{object 29 0}} << +>>stream + + +endstream +endobj +{{object 30 0}} << +>>stream + +endstream +endobj +{{xref}} +trailer << + /Root 1 0 R +>> +{{startxref}} +%%EOF diff --git a/testing/resources/javascript/bug_679643_expected.txt b/testing/resources/javascript/bug_679643_expected.txt new file mode 100644 index 0000000000..36d4a31344 --- /dev/null +++ b/testing/resources/javascript/bug_679643_expected.txt @@ -0,0 +1,3 @@ +Alert: Starting ... +Alert: Firing ... +Alert: failed: Annot.name: Object no longer exists. -- cgit v1.2.3