Do not check pointers before deleting them.

xfa/fxfa/parser/xfa_script_imp.cpp

 // Copyright 2014 PDFium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
 
 #include "xfa/fxfa/parser/xfa_script_imp.h"
 
 #include "core/fxcrt/include/fx_ext.h"
 #include "xfa/fxfa/app/xfa_ffnotify.h"
@@ skipping 35 matching lines @@
   m_mapXFAToHValue.RemoveAll();
   ReleaseVariablesMap();
   if (m_hFM2JSContext) {
     XFA_FM2JS_ContextRelease(m_hFM2JSContext);
     m_hFM2JSContext = NULL;
   }
   if (m_hJsContext) {
     FXJSE_Context_Release(m_hJsContext);
     m_hJsContext = NULL;
   }
-  if (m_pResolveProcessor) {
-    delete m_pResolveProcessor;
-    m_pResolveProcessor = NULL;
-  }
+  delete m_pResolveProcessor;
   m_upObjectArray.RemoveAll();
   for (int32_t i = 0; i < m_CacheListArray.GetSize(); i++)
     delete m_CacheListArray[i];
 }
 void CXFA_ScriptContext::Initialize(FXJSE_HRUNTIME hRuntime) {
   m_hJsRuntime = hRuntime;
   DefineJsContext();
   DefineJsClass();
   m_pResolveProcessor = new CXFA_ResolveProcessor;
 }
@@ skipping 321 matching lines @@
   m_JsGlobalClass.dynMethodCall = CXFA_ScriptContext::NormalMethodCall;
   m_hJsContext = FXJSE_Context_Create(m_hJsRuntime, &m_JsGlobalClass,
                                       m_pDocument->GetRoot());
   RemoveBuiltInObjs(m_hJsContext);
   FXJSE_Context_EnableCompatibleMode(
       m_hJsContext, FXJSE_COMPATIBLEMODEFLAG_CONSTRUCTOREXTRAMETHODS);
 }
 FXJSE_HCONTEXT CXFA_ScriptContext::CreateVariablesContext(
     CXFA_Node* pScriptNode,
     CXFA_Node* pSubform) {
-  if (pScriptNode == NULL || pSubform == NULL) {
-    return NULL;
-  }
+  if (!pScriptNode || !pSubform)
+    return nullptr;
+
   if (m_mapVariableToHValue.GetCount() == 0) {
-    m_JsGlobalVariablesClass.constructor = NULL;
+    m_JsGlobalVariablesClass.constructor = nullptr;
     m_JsGlobalVariablesClass.name = "XFAScriptObject";
     m_JsGlobalVariablesClass.propNum = 0;
-    m_JsGlobalVariablesClass.properties = NULL;
+    m_JsGlobalVariablesClass.properties = nullptr;
     m_JsGlobalVariablesClass.methNum = 0;
-    m_JsGlobalVariablesClass.methods = NULL;
+    m_JsGlobalVariablesClass.methods = nullptr;
     m_JsGlobalVariablesClass.dynPropGetter =
         CXFA_ScriptContext::GlobalPropertyGetter;
     m_JsGlobalVariablesClass.dynPropSetter =
         CXFA_ScriptContext::GlobalPropertySetter;
     m_JsGlobalVariablesClass.dynPropTypeGetter =
         CXFA_ScriptContext::NormalPropTypeGetter;
-    m_JsGlobalVariablesClass.dynPropDeleter = NULL;
+    m_JsGlobalVariablesClass.dynPropDeleter = nullptr;
     m_JsGlobalVariablesClass.dynMethodCall =
         CXFA_ScriptContext::NormalMethodCall;
   }
-  CXFA_ThisProxy* lpVariableNode = new CXFA_ThisProxy(pSubform, pScriptNode);
-  FXJSE_HCONTEXT hVariablesContext = FXJSE_Context_Create(
-      m_hJsRuntime, &m_JsGlobalVariablesClass, (CXFA_Object*)lpVariableNode);
+  FXJSE_HCONTEXT hVariablesContext =
+      FXJSE_Context_Create(m_hJsRuntime, &m_JsGlobalVariablesClass,
+                           new CXFA_ThisProxy(pSubform, pScriptNode));
   RemoveBuiltInObjs(hVariablesContext);
   FXJSE_Context_EnableCompatibleMode(
       hVariablesContext, FXJSE_COMPATIBLEMODEFLAG_CONSTRUCTOREXTRAMETHODS);
   m_mapVariableToHValue.SetAt(pScriptNode, hVariablesContext);
   return hVariablesContext;
 }
 CXFA_Object* CXFA_ScriptContext::GetVariablesThis(CXFA_Object* pObject,
                                                   FX_BOOL bScriptNode) {
   if (pObject->GetObjectType() == XFA_OBJECTTYPE_VariablesThis) {
     return bScriptNode ? ((CXFA_ThisProxy*)pObject)->GetScriptNode()
                        : ((CXFA_ThisProxy*)pObject)->GetThisNode();
   }
   return pObject;
 }
+
 FX_BOOL CXFA_ScriptContext::RunVariablesScript(CXFA_Node* pScriptNode) {
-  if (pScriptNode == NULL) {
+  if (!pScriptNode)
     return FALSE;
-  }
-  if (pScriptNode->GetClassID() == XFA_ELEMENT_Script) {
-    CXFA_Node* pParent = pScriptNode->GetNodeItem(XFA_NODEITEM_Parent);
-    if (!pParent || pParent->GetClassID() != XFA_ELEMENT_Variables) {
-      return FALSE;
-    }
-    if (m_mapVariableToHValue.GetValueAt(pScriptNode)) {
-      return TRUE;
-    }
-    CXFA_Node* pTextNode = pScriptNode->GetNodeItem(XFA_NODEITEM_FirstChild);
-    if (!pTextNode) {
-      return FALSE;
-    }
-    CFX_WideStringC wsScript;
-    if (!pTextNode->TryCData(XFA_ATTRIBUTE_Value, wsScript)) {
-      return FALSE;
-    }
-    CFX_ByteString btScript =
-        FX_UTF8Encode(wsScript.c_str(), wsScript.GetLength());
-    FXJSE_HVALUE hRetValue = FXJSE_Value_Create(m_hJsRuntime);
-    CXFA_Node* pThisObject = pParent->GetNodeItem(XFA_NODEITEM_Parent);
-    FXJSE_HCONTEXT hVariablesContext =
-        CreateVariablesContext(pScriptNode, pThisObject);
-    CXFA_Object* pOriginalObject = m_pThisObject;
-    m_pThisObject = pThisObject;
-    FX_BOOL bRet =
-        FXJSE_ExecuteScript(hVariablesContext, btScript.c_str(), hRetValue);
-    m_pThisObject = pOriginalObject;
-    FXJSE_Value_Release(hRetValue);
-    return bRet;
-  }
-  return TRUE;
+
+  if (pScriptNode->GetClassID() != XFA_ELEMENT_Script)
+    return TRUE;
+
+  CXFA_Node* pParent = pScriptNode->GetNodeItem(XFA_NODEITEM_Parent);
+  if (!pParent || pParent->GetClassID() != XFA_ELEMENT_Variables)
+    return FALSE;
+
+  if (m_mapVariableToHValue.GetValueAt(pScriptNode))
+    return TRUE;
+
+  CXFA_Node* pTextNode = pScriptNode->GetNodeItem(XFA_NODEITEM_FirstChild);
+  if (!pTextNode)
+    return FALSE;
+
+  CFX_WideStringC wsScript;
+  if (!pTextNode->TryCData(XFA_ATTRIBUTE_Value, wsScript))
+    return FALSE;
+
+  CFX_ByteString btScript =
+      FX_UTF8Encode(wsScript.c_str(), wsScript.GetLength());
+  FXJSE_HVALUE hRetValue = FXJSE_Value_Create(m_hJsRuntime);
+  CXFA_Node* pThisObject = pParent->GetNodeItem(XFA_NODEITEM_Parent);
+  FXJSE_HCONTEXT hVariablesContext =
+      CreateVariablesContext(pScriptNode, pThisObject);
+  CXFA_Object* pOriginalObject = m_pThisObject;
+  m_pThisObject = pThisObject;
+  FX_BOOL bRet =
+      FXJSE_ExecuteScript(hVariablesContext, btScript.c_str(), hRetValue);
+  m_pThisObject = pOriginalObject;
+  FXJSE_Value_Release(hRetValue);
+  return bRet;
 }
+
 FX_BOOL CXFA_ScriptContext::QueryVariableHValue(
     CXFA_Node* pScriptNode,
     const CFX_ByteStringC& szPropName,
     FXJSE_HVALUE hValue,
     FX_BOOL bGetter) {
-  if (!pScriptNode || pScriptNode->GetClassID() != XFA_ELEMENT_Script) {
+  if (!pScriptNode || pScriptNode->GetClassID() != XFA_ELEMENT_Script)
     return FALSE;
+
+  CXFA_Node* variablesNode = pScriptNode->GetNodeItem(XFA_NODEITEM_Parent);
+  if (!variablesNode || variablesNode->GetClassID() != XFA_ELEMENT_Variables)
+    return FALSE;
+
+  void* lpVariables = m_mapVariableToHValue.GetValueAt(pScriptNode);
+  if (!lpVariables)
+    return FALSE;
+
+  FX_BOOL bRes = FALSE;
+  FXJSE_HCONTEXT hVariableContext = (FXJSE_HCONTEXT)lpVariables;
+  FXJSE_HVALUE hObject = FXJSE_Context_GetGlobalObject(hVariableContext);
+  FXJSE_HVALUE hVariableValue = FXJSE_Value_Create(m_hJsRuntime);
+  if (!bGetter) {
+    FXJSE_Value_SetObjectOwnProp(hObject, szPropName, hValue);
+    bRes = TRUE;
+  } else if (FXJSE_Value_ObjectHasOwnProp(hObject, szPropName, FALSE)) {
+    FXJSE_Value_GetObjectProp(hObject, szPropName, hVariableValue);
+    if (FXJSE_Value_IsFunction(hVariableValue))
+      FXJSE_Value_SetFunctionBind(hValue, hVariableValue, hObject);
+    else if (bGetter)
+      FXJSE_Value_Set(hValue, hVariableValue);
+    else
+      FXJSE_Value_Set(hVariableValue, hValue);
+    bRes = TRUE;
   }
-  CXFA_Node* variablesNode = pScriptNode->GetNodeItem(XFA_NODEITEM_Parent);
-  if (!variablesNode || variablesNode->GetClassID() != XFA_ELEMENT_Variables) {
-    return FALSE;
-  }
-  FX_BOOL bRes = FALSE;
-  void* lpVariables = m_mapVariableToHValue.GetValueAt(pScriptNode);
-  if (lpVariables) {
-    FXJSE_HCONTEXT hVariableContext = (FXJSE_HCONTEXT)lpVariables;
-    FXJSE_HVALUE hObject = FXJSE_Context_GetGlobalObject(hVariableContext);
-    FXJSE_HVALUE hVariableValue = FXJSE_Value_Create(m_hJsRuntime);
-    if (!bGetter) {
-      FXJSE_Value_SetObjectOwnProp(hObject, szPropName, hValue);
-      bRes = TRUE;
-    } else if (FXJSE_Value_ObjectHasOwnProp(hObject, szPropName, FALSE)) {
-      FXJSE_Value_GetObjectProp(hObject, szPropName, hVariableValue);
-      if (FXJSE_Value_IsFunction(hVariableValue)) {
-        FXJSE_Value_SetFunctionBind(hValue, hVariableValue, hObject);
-      } else if (bGetter) {
-        FXJSE_Value_Set(hValue, hVariableValue);
-      } else {
-        FXJSE_Value_Set(hVariableValue, hValue);
-      }
-      bRes = TRUE;
-    }
-    FXJSE_Value_Release(hVariableValue);
-    FXJSE_Value_Release(hObject);
-  }
+  FXJSE_Value_Release(hVariableValue);
+  FXJSE_Value_Release(hObject);
   return bRes;
 }
+
 void CXFA_ScriptContext::ReleaseVariablesMap() {
   FX_POSITION ps = m_mapVariableToHValue.GetStartPosition();
   while (ps) {
     CXFA_Object* pScriptNode;
-    FXJSE_HCONTEXT hVariableContext;
+    FXJSE_HCONTEXT hVariableContext = nullptr;
     m_mapVariableToHValue.GetNextAssoc(ps, pScriptNode, hVariableContext);
     FXJSE_HVALUE hObject = FXJSE_Context_GetGlobalObject(hVariableContext);
-    CXFA_Object* lpCurNode = (CXFA_Object*)FXJSE_Value_ToObject(hObject, NULL);
-    if (lpCurNode) {
-      delete (CXFA_ThisProxy*)lpCurNode;
-      lpCurNode = NULL;
-    }
+    delete reinterpret_cast<CXFA_ThisProxy*>(

[Tom Sepez, 2016/04/28 23:07:55]

static_cast not good enough (void return?)  Also why CXFA_ThisProxy* and not
CXFA_Object* like before?

[Lei Zhang, 2016/04/28 23:43:53]

static_cast is good enough. The object gets casted twice in the original code.

[Tom Sepez, 2016/04/28 23:47:39]

Acknowledged.

+        FXJSE_Value_ToObject(hObject, nullptr));
     FXJSE_Value_Release(hObject);
     FXJSE_Context_Release(hVariableContext);
-    hVariableContext = NULL;
   }
   m_mapVariableToHValue.RemoveAll();
 }
+
 void CXFA_ScriptContext::DefineJsClass() {
   m_JsNormalClass.constructor = NULL;
   m_JsNormalClass.name = "XFAObject";
   m_JsNormalClass.propNum = 0;
   m_JsNormalClass.properties = NULL;
   m_JsNormalClass.methNum = 0;
   m_JsNormalClass.methods = NULL;
   m_JsNormalClass.dynPropGetter = CXFA_ScriptContext::NormalPropertyGetter;
   m_JsNormalClass.dynPropSetter = CXFA_ScriptContext::NormalPropertySetter;
   m_JsNormalClass.dynPropTypeGetter = CXFA_ScriptContext::NormalPropTypeGetter;
@@ skipping 225 matching lines @@
   }
 }
 void CXFA_ScriptContext::AddNodesOfRunScript(CXFA_Node* pNode) {
   if (!m_pScriptNodeArray) {
     return;
   }
   if (m_pScriptNodeArray->Find(pNode) == -1) {
     m_pScriptNodeArray->Add(pNode);
   }
 }