Fix uninitialized memory read in CJS_Object::GetEmbedObject()
The expected way to create native PDFium objects for JS is via
the NewFxDynamicObject() call in C++, but that doesn't mean that the
corresponding constructors won't be called from JS. In that case,
the internal fields will be uninitialized, and subsequent method
calls may try to use them.
Add a constructor callback for all PDFium objects that nulls out
these fields (shame that v8 doesn't do this by default, but probably
saves some cycles). Then ensure that we check for this possibility
in all the places it might turn up.
Conversely, if we've just gotten a successful return from
NewFxDynamicObject(), we know the CJS_Object/EmbedObj are good,
so avoid checking there.
BUG=695826
Change-Id: Iadad644c4af937def967ddc83daac1dad7544d69
Reviewed-on: https://pdfium-review.googlesource.com/2839
Reviewed-by: dsinclair <dsinclair@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
diff --git a/fpdfsdk/javascript/Document.cpp b/fpdfsdk/javascript/Document.cpp
index 536d654..6928a06 100644
--- a/fpdfsdk/javascript/Document.cpp
+++ b/fpdfsdk/javascript/Document.cpp
@@ -299,11 +299,13 @@
v8::Local<v8::Object> pFieldObj =
pRuntime->NewFxDynamicObj(CJS_Field::g_nObjDefnID);
+ if (pFieldObj.IsEmpty())
+ return false;
+
CJS_Field* pJSField =
static_cast<CJS_Field*>(pRuntime->GetObjectPrivate(pFieldObj));
Field* pField = static_cast<Field*>(pJSField->GetEmbedObject());
pField->AttachField(this, wideName);
-
vRet = CJS_Value(pRuntime, pJSField);
return true;
}
@@ -1111,13 +1113,7 @@
CJS_Annot* pJS_Annot =
static_cast<CJS_Annot*>(pRuntime->GetObjectPrivate(pObj));
- if (!pJS_Annot)
- return false;
-
Annot* pAnnot = static_cast<Annot*>(pJS_Annot->GetEmbedObject());
- if (!pAnnot)
- return false;
-
pAnnot->SetSDKAnnot(pSDKBAAnnot);
vRet = CJS_Value(pRuntime, pJS_Annot);
return true;
@@ -1155,13 +1151,7 @@
CJS_Annot* pJS_Annot =
static_cast<CJS_Annot*>(pRuntime->GetObjectPrivate(pObj));
- if (!pJS_Annot)
- return false;
-
Annot* pAnnot = static_cast<Annot*>(pJS_Annot->GetEmbedObject());
- if (!pAnnot)
- return false;
-
pAnnot->SetSDKAnnot(static_cast<CPDFSDK_BAAnnot*>(pSDKAnnotCur.Get()));
annots.SetElement(pRuntime, i, CJS_Value(pRuntime, pJS_Annot));
}
@@ -1258,13 +1248,7 @@
CJS_Icon* pJS_Icon =
static_cast<CJS_Icon*>(pRuntime->GetObjectPrivate(pObj));
- if (!pJS_Icon)
- return false;
-
Icon* pIcon = static_cast<Icon*>(pJS_Icon->GetEmbedObject());
- if (!pIcon)
- return false;
-
pIcon->SetIconName(pIconElement->IconName);
Icons.SetElement(pRuntime, i++, CJS_Value(pRuntime, pJS_Icon));
}
@@ -1297,13 +1281,7 @@
CJS_Icon* pJS_Icon =
static_cast<CJS_Icon*>(pRuntime->GetObjectPrivate(pObj));
- if (!pJS_Icon)
- return false;
-
- Icon* pIcon = (Icon*)pJS_Icon->GetEmbedObject();
- if (!pIcon)
- return false;
-
+ Icon* pIcon = static_cast<Icon*>(pJS_Icon->GetEmbedObject());
pIcon->SetIconName(swIconName);
vRet = CJS_Value(pRuntime, pJS_Icon);
return true;
@@ -1473,6 +1451,8 @@
CFX_WideString& sError) {
v8::Local<v8::Object> pRetObj =
pRuntime->NewFxDynamicObj(CJS_PrintParamsObj::g_nObjDefnID);
+ if (pRetObj.IsEmpty())
+ return false;
// Not implemented yet.
diff --git a/fpdfsdk/javascript/Field.cpp b/fpdfsdk/javascript/Field.cpp
index 92b473b..f37b3d4 100644
--- a/fpdfsdk/javascript/Field.cpp
+++ b/fpdfsdk/javascript/Field.cpp
@@ -2835,7 +2835,8 @@
v8::Local<v8::Object> pObj =
pRuntime->NewFxDynamicObj(CJS_Icon::g_nObjDefnID);
- ASSERT(pObj.IsEmpty() == false);
+ if (pObj.IsEmpty())
+ return false;
CJS_Icon* pJS_Icon = static_cast<CJS_Icon*>(pRuntime->GetObjectPrivate(pObj));
vRet = CJS_Value(pRuntime, pJS_Icon);
@@ -2966,7 +2967,8 @@
for (const auto& pStr : swSort) {
v8::Local<v8::Object> pObj =
pRuntime->NewFxDynamicObj(CJS_Field::g_nObjDefnID);
- ASSERT(!pObj.IsEmpty());
+ if (pObj.IsEmpty())
+ return false;
CJS_Field* pJSField =
static_cast<CJS_Field*>(pRuntime->GetObjectPrivate(pObj));
diff --git a/fpdfsdk/javascript/JS_Define.h b/fpdfsdk/javascript/JS_Define.h
index ee7448c..375ca3a 100644
--- a/fpdfsdk/javascript/JS_Define.h
+++ b/fpdfsdk/javascript/JS_Define.h
@@ -45,6 +45,8 @@
return;
CJS_Object* pJSObj =
static_cast<CJS_Object*>(pRuntime->GetObjectPrivate(info.Holder()));
+ if (!pJSObj)
+ return;
C* pObj = reinterpret_cast<C*>(pJSObj->GetEmbedObject());
CFX_WideString sError;
CJS_PropValue value(pRuntime);
@@ -69,6 +71,8 @@
return;
CJS_Object* pJSObj =
static_cast<CJS_Object*>(pRuntime->GetObjectPrivate(info.Holder()));
+ if (!pJSObj)
+ return;
C* pObj = reinterpret_cast<C*>(pJSObj->GetEmbedObject());
CFX_WideString sError;
CJS_PropValue propValue(pRuntime, CJS_Value(pRuntime, value));
@@ -111,6 +115,8 @@
}
CJS_Object* pJSObj =
static_cast<CJS_Object*>(pRuntime->GetObjectPrivate(info.Holder()));
+ if (!pJSObj)
+ return;
C* pObj = reinterpret_cast<C*>(pJSObj->GetEmbedObject());
CFX_WideString sError;
CJS_Value valueRes(pRuntime);
@@ -210,33 +216,31 @@
static JSPropertySpec PropertySpecs[]; \
static JSMethodSpec MethodSpecs[];
-#define IMPLEMENT_JS_CLASS_RICH_PART(js_class_name, class_alternate, \
- class_name) \
- void js_class_name::JSConstructor(CFXJS_Engine* pEngine, \
- v8::Local<v8::Object> obj) { \
- CJS_Object* pObj = new js_class_name(obj); \
- pObj->SetEmbedObject(new class_alternate(pObj)); \
- pEngine->SetObjectPrivate(obj, (void*)pObj); \
- pObj->InitInstance(static_cast<CJS_Runtime*>(pEngine)); \
- } \
- void js_class_name::JSDestructor(CFXJS_Engine* pEngine, \
- v8::Local<v8::Object> obj) { \
- js_class_name* pObj = \
- static_cast<js_class_name*>(pEngine->GetObjectPrivate(obj)); \
- delete pObj; \
- } \
- void js_class_name::DefineProps(CFXJS_Engine* pEngine) { \
- for (size_t i = 0; i < FX_ArraySize(PropertySpecs) - 1; ++i) { \
- pEngine->DefineObjProperty(g_nObjDefnID, PropertySpecs[i].pName, \
- PropertySpecs[i].pPropGet, \
- PropertySpecs[i].pPropPut); \
- } \
- } \
- void js_class_name::DefineMethods(CFXJS_Engine* pEngine) { \
- for (size_t i = 0; i < FX_ArraySize(MethodSpecs) - 1; ++i) { \
- pEngine->DefineObjMethod(g_nObjDefnID, MethodSpecs[i].pName, \
- MethodSpecs[i].pMethodCall); \
- } \
+#define IMPLEMENT_JS_CLASS_RICH_PART(js_class_name, class_alternate, \
+ class_name) \
+ void js_class_name::JSConstructor(CFXJS_Engine* pEngine, \
+ v8::Local<v8::Object> obj) { \
+ CJS_Object* pObj = new js_class_name(obj); \
+ pObj->SetEmbedObject(new class_alternate(pObj)); \
+ pEngine->SetObjectPrivate(obj, (void*)pObj); \
+ pObj->InitInstance(static_cast<CJS_Runtime*>(pEngine)); \
+ } \
+ void js_class_name::JSDestructor(CFXJS_Engine* pEngine, \
+ v8::Local<v8::Object> obj) { \
+ delete static_cast<js_class_name*>(pEngine->GetObjectPrivate(obj)); \
+ } \
+ void js_class_name::DefineProps(CFXJS_Engine* pEngine) { \
+ for (size_t i = 0; i < FX_ArraySize(PropertySpecs) - 1; ++i) { \
+ pEngine->DefineObjProperty(g_nObjDefnID, PropertySpecs[i].pName, \
+ PropertySpecs[i].pPropGet, \
+ PropertySpecs[i].pPropPut); \
+ } \
+ } \
+ void js_class_name::DefineMethods(CFXJS_Engine* pEngine) { \
+ for (size_t i = 0; i < FX_ArraySize(MethodSpecs) - 1; ++i) { \
+ pEngine->DefineObjMethod(g_nObjDefnID, MethodSpecs[i].pName, \
+ MethodSpecs[i].pMethodCall); \
+ } \
}
// Special JS classes implement methods, props, and queries, but not consts.
@@ -310,12 +314,18 @@
const v8::PropertyCallbackInfo<v8::Integer>& info) {
CJS_Runtime* pRuntime =
CJS_Runtime::CurrentRuntimeFromIsolate(info.GetIsolate());
+ if (!pRuntime)
+ return;
+
+ CJS_Object* pJSObj =
+ static_cast<CJS_Object*>(pRuntime->GetObjectPrivate(info.Holder()));
+ if (!pJSObj)
+ return;
+
+ Alt* pObj = reinterpret_cast<Alt*>(pJSObj->GetEmbedObject());
v8::String::Utf8Value utf8_value(property);
CFX_WideString propname = CFX_WideString::FromUTF8(
CFX_ByteStringC(*utf8_value, utf8_value.length()));
- CJS_Object* pJSObj =
- static_cast<CJS_Object*>(pRuntime->GetObjectPrivate(info.Holder()));
- Alt* pObj = reinterpret_cast<Alt*>(pJSObj->GetEmbedObject());
bool bRet = pObj->QueryProperty(propname.c_str());
info.GetReturnValue().Set(bRet ? 4 : 0);
}
@@ -328,8 +338,12 @@
CJS_Runtime::CurrentRuntimeFromIsolate(info.GetIsolate());
if (!pRuntime)
return;
+
CJS_Object* pJSObj =
static_cast<CJS_Object*>(pRuntime->GetObjectPrivate(info.Holder()));
+ if (!pJSObj)
+ return;
+
Alt* pObj = reinterpret_cast<Alt*>(pJSObj->GetEmbedObject());
v8::String::Utf8Value utf8_value(property);
CFX_WideString propname = CFX_WideString::FromUTF8(
@@ -353,8 +367,12 @@
CJS_Runtime::CurrentRuntimeFromIsolate(info.GetIsolate());
if (!pRuntime)
return;
+
CJS_Object* pJSObj =
static_cast<CJS_Object*>(pRuntime->GetObjectPrivate(info.Holder()));
+ if (!pJSObj)
+ return;
+
Alt* pObj = reinterpret_cast<Alt*>(pJSObj->GetEmbedObject());
v8::String::Utf8Value utf8_value(property);
CFX_WideString propname = CFX_WideString::FromUTF8(
@@ -375,8 +393,12 @@
CJS_Runtime::CurrentRuntimeFromIsolate(info.GetIsolate());
if (!pRuntime)
return;
+
CJS_Object* pJSObj =
static_cast<CJS_Object*>(pRuntime->GetObjectPrivate(info.Holder()));
+ if (!pJSObj)
+ return;
+
Alt* pObj = reinterpret_cast<Alt*>(pJSObj->GetEmbedObject());
v8::String::Utf8Value utf8_value(property);
CFX_WideString propname = CFX_WideString::FromUTF8(
diff --git a/fpdfsdk/javascript/JS_EventHandler.cpp b/fpdfsdk/javascript/JS_EventHandler.cpp
index cec7735..bd1c8e2 100644
--- a/fpdfsdk/javascript/JS_EventHandler.cpp
+++ b/fpdfsdk/javascript/JS_EventHandler.cpp
@@ -590,22 +590,25 @@
CJS_Runtime* pRuntime = m_pJSEventContext->GetJSRuntime();
v8::Local<v8::Object> pDocObj =
pRuntime->NewFxDynamicObj(CJS_Document::g_nObjDefnID);
- ASSERT(!pDocObj.IsEmpty());
+ if (pDocObj.IsEmpty())
+ return nullptr;
v8::Local<v8::Object> pFieldObj =
pRuntime->NewFxDynamicObj(CJS_Field::g_nObjDefnID);
- ASSERT(!pFieldObj.IsEmpty());
+ if (pFieldObj.IsEmpty())
+ return nullptr;
CJS_Document* pJSDocument =
static_cast<CJS_Document*>(pRuntime->GetObjectPrivate(pDocObj));
- Document* pDocument = (Document*)pJSDocument->GetEmbedObject();
+ CJS_Field* pJSField =
+ static_cast<CJS_Field*>(pRuntime->GetObjectPrivate(pFieldObj));
+
+ Document* pDocument = static_cast<Document*>(pJSDocument->GetEmbedObject());
pDocument->SetFormFillEnv(m_pTargetFormFillEnv
? m_pTargetFormFillEnv.Get()
: m_pJSEventContext->GetFormFillEnv());
- CJS_Field* pJSField =
- static_cast<CJS_Field*>(pRuntime->GetObjectPrivate(pFieldObj));
- Field* pField = (Field*)pJSField->GetEmbedObject();
+ Field* pField = static_cast<Field*>(pJSField->GetEmbedObject());
pField->AttachField(pDocument, m_strSourceName);
return pField;
}
@@ -614,22 +617,25 @@
CJS_Runtime* pRuntime = m_pJSEventContext->GetJSRuntime();
v8::Local<v8::Object> pDocObj =
pRuntime->NewFxDynamicObj(CJS_Document::g_nObjDefnID);
- ASSERT(!pDocObj.IsEmpty());
+ if (pDocObj.IsEmpty())
+ return nullptr;
v8::Local<v8::Object> pFieldObj =
pRuntime->NewFxDynamicObj(CJS_Field::g_nObjDefnID);
- ASSERT(!pFieldObj.IsEmpty());
+ if (pFieldObj.IsEmpty())
+ return nullptr;
CJS_Document* pJSDocument =
static_cast<CJS_Document*>(pRuntime->GetObjectPrivate(pDocObj));
- Document* pDocument = (Document*)pJSDocument->GetEmbedObject();
+ CJS_Field* pJSField =
+ static_cast<CJS_Field*>(pRuntime->GetObjectPrivate(pFieldObj));
+
+ Document* pDocument = static_cast<Document*>(pJSDocument->GetEmbedObject());
pDocument->SetFormFillEnv(m_pTargetFormFillEnv
? m_pTargetFormFillEnv.Get()
: m_pJSEventContext->GetFormFillEnv());
- CJS_Field* pJSField =
- static_cast<CJS_Field*>(pRuntime->GetObjectPrivate(pFieldObj));
- Field* pField = (Field*)pJSField->GetEmbedObject();
+ Field* pField = static_cast<Field*>(pJSField->GetEmbedObject());
pField->AttachField(pDocument, m_strTargetName);
return pField;
}
diff --git a/fpdfsdk/javascript/app.cpp b/fpdfsdk/javascript/app.cpp
index 0c75f6b..3a8bf08 100644
--- a/fpdfsdk/javascript/app.cpp
+++ b/fpdfsdk/javascript/app.cpp
@@ -215,13 +215,11 @@
CJS_Document* pJSDocument = nullptr;
v8::Local<v8::Object> pObj = pRuntime->GetThisObj();
- if (CFXJS_Engine::GetObjDefnID(pObj) == CJS_Document::g_nObjDefnID) {
+ if (CFXJS_Engine::GetObjDefnID(pObj) == CJS_Document::g_nObjDefnID)
pJSDocument = static_cast<CJS_Document*>(pRuntime->GetObjectPrivate(pObj));
- }
CJS_Array aDocs;
aDocs.SetElement(pRuntime, 0, CJS_Value(pRuntime, pJSDocument));
-
if (aDocs.GetLength(pRuntime) > 0)
vp << aDocs;
else
@@ -472,6 +470,9 @@
v8::Local<v8::Object> pRetObj =
pRuntime->NewFxDynamicObj(CJS_TimerObj::g_nObjDefnID);
+ if (pRetObj.IsEmpty())
+ return false;
+
CJS_TimerObj* pJS_TimerObj =
static_cast<CJS_TimerObj*>(pRuntime->GetObjectPrivate(pRetObj));
TimerObj* pTimerObj = static_cast<TimerObj*>(pJS_TimerObj->GetEmbedObject());
@@ -504,13 +505,13 @@
v8::Local<v8::Object> pRetObj =
pRuntime->NewFxDynamicObj(CJS_TimerObj::g_nObjDefnID);
+ if (pRetObj.IsEmpty())
+ return false;
CJS_TimerObj* pJS_TimerObj =
static_cast<CJS_TimerObj*>(pRuntime->GetObjectPrivate(pRetObj));
-
TimerObj* pTimerObj = static_cast<TimerObj*>(pJS_TimerObj->GetEmbedObject());
pTimerObj->SetTimer(timerRef);
-
vRet = CJS_Value(pRuntime, pRetObj);
return true;
}
diff --git a/fxjs/fxjs_v8.cpp b/fxjs/fxjs_v8.cpp
index 053db07..0c211a1 100644
--- a/fxjs/fxjs_v8.cpp
+++ b/fxjs/fxjs_v8.cpp
@@ -56,6 +56,12 @@
v8::Local<v8::FunctionTemplate> fun = v8::FunctionTemplate::New(isolate);
fun->InstanceTemplate()->SetInternalFieldCount(2);
+ fun->SetCallHandler([](const v8::FunctionCallbackInfo<v8::Value>& info) {
+ v8::Local<v8::Object> holder = info.Holder();
+ ASSERT(holder->InternalFieldCount() == 2);
+ holder->SetAlignedPointerInInternalField(0, nullptr);
+ holder->SetAlignedPointerInInternalField(1, nullptr);
+ });
if (eObjType == FXJSOBJTYPE_GLOBAL) {
fun->InstanceTemplate()->Set(
v8::Symbol::GetToStringTag(isolate),
diff --git a/testing/resources/javascript/bug_695826.in b/testing/resources/javascript/bug_695826.in
new file mode 100644
index 0000000..b20908b
--- /dev/null
+++ b/testing/resources/javascript/bug_695826.in
@@ -0,0 +1,47 @@
+{{header}}
+{{object 1 0}} <<
+ /Type /Catalog
+ /Pages 2 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 <</F1 15 0 R>>
+ >>
+ /Contents [21 0 R]
+ /MediaBox [0 0 612 792]
+>>
+% OpenAction action
+{{object 10 0}} <<
+ /Type /Action
+ /S /JavaScript
+ /JS 11 0 R
+>>
+endobj
+% JS program to exexute
+{{object 11 0}} <<
+>>
+stream
+var obj = new this.constructor;
+obj.author = 3;
+app.alert('Done!');
+endstream
+endobj
+{{xref}}
+trailer <<
+ /Root 1 0 R
+>>
+{{startxref}}
+%%EOF
diff --git a/testing/resources/javascript/bug_695826_expected.txt b/testing/resources/javascript/bug_695826_expected.txt
new file mode 100644
index 0000000..5bb6e85
--- /dev/null
+++ b/testing/resources/javascript/bug_695826_expected.txt
@@ -0,0 +1 @@
+Alert: Done!
