Do not check pointers before deleting them.

xfa/fxfa/parser/xfa_script_imp.cpp

Index: xfa/fxfa/parser/xfa_script_imp.cpp
diff --git a/xfa/fxfa/parser/xfa_script_imp.cpp b/xfa/fxfa/parser/xfa_script_imp.cpp
index ea16fd1e7c2919f831e9181d0832d4b7ee4a1b2e..4a9fa785d177e9104d7a8893b1160178df810fcc 100644
--- a/xfa/fxfa/parser/xfa_script_imp.cpp
+++ b/xfa/fxfa/parser/xfa_script_imp.cpp
@@ -53,10 +53,7 @@ CXFA_ScriptContext::~CXFA_ScriptContext() {
     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];
@@ -398,29 +395,29 @@ void CXFA_ScriptContext::DefineJsContext() {
 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);
@@ -435,97 +432,97 @@ CXFA_Object* CXFA_ScriptContext::GetVariablesThis(CXFA_Object* pObject,
   }
   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) {
+  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);
-  }
+  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;
+  }
+  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";