Tidy up app::response().

fpdfsdk/src/javascript/app.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 "../../include/javascript/JavaScript.h"
 #include "../../include/javascript/IJavaScript.h"
 #include "../../include/javascript/JS_Define.h"
 #include "../../include/javascript/JS_Object.h"
@@ skipping 1019 matching lines @@
 FX_BOOL app::response(OBJ_METHOD_PARAMS)
 {
         CFX_WideString swQuestion = L"";
         CFX_WideString swLabel = L"";
 #ifndef FOXIT_CHROME_BUILD
         CFX_WideString swTitle = L"Foxit";
 #else
         CFX_WideString swTitle = L"PDF";
 #endif
         CFX_WideString swDefault = L"";
-        CFX_WideString swResponse = L"";
         bool bPassWord = false;
-        
+
         v8::Isolate* isolate = GetIsolate(cc);
-        
-        int iLength = params.size();    
+
+        int iLength = params.size();
         if (iLength > 0 && params[0].GetType() == VT_object)
         {
-                
                 JSObject pObj = (JSObject )params[0];
                 v8::Handle<v8::Value> pValue = JS_GetObjectElement(isolate,pObj,L"cQuestion");
                         swQuestion = CJS_Value(isolate,pValue,GET_VALUE_TYPE(pValue)).operator CFX_WideString();
 
                 pValue = JS_GetObjectElement(isolate,pObj,L"cTitle");
                         swTitle = CJS_Value(isolate,pValue,GET_VALUE_TYPE(pValue)).operator CFX_WideString();
 
                 pValue = JS_GetObjectElement(isolate,pObj,L"cDefault");
                         swDefault = CJS_Value(isolate,pValue,GET_VALUE_TYPE(pValue)).operator CFX_WideString();
 
@@ skipping 36 matching lines @@
                         break;
                 }
         }
 
         CJS_Context* pContext = (CJS_Context *)cc;
         ASSERT(pContext != NULL);
 
         CPDFDoc_Environment* pApp = pContext->GetReaderApp();
         ASSERT(pApp != NULL);
         int nLength = 2048;
         char* pBuff = new char[nLength];

[jun_fang, 2014/07/29 04:11:30]

nLength is the length of content. Doesn't include the last two chars '\0' in the
following wide string. So allocate 2 more chars here. 
char* pBuff = new char[nLength+2];

Initialize all bytes with '\0' to indicate the ending char when the buffer is
saved with contents.
memset(pBuff, 0, nLength+2);

[Tom Sepez, 2014/07/29 16:55:57]

Yeah, I'm having a hard time reconciling this with the description of the API in
fpdfformfill.h that says "Number of bytes the user input text consumes, not
including trailing zeros. If the text exceed 2048 bytes, the exceeded part will
be ignored.

Does this mean that there are trailing zeros written to the buffer by the method
and they're just not counted?  Or can we not expect any to be written unless we
put them there in the caller?  If the input is 2048 bytes, will we get 2048
bytes plus an out-of-bounds trailing (0 0)? Or will we get 2048 bytes and no
zeros?  Or will we get 2046 bytes and trailing (0 0).

[jun_fang, 2014/07/29 17:31:47]

In theory, there is no difference between 2046 plus trailing (0 0) and 2048 plus
trailing (0 0). If we use 2046 plus trailing (0 0), all length used as
parameters should be changed to length-2, it's not as popular as 2048 plus
trailing (0 0). People always think that length represents the length of data.
In the latter way (2048 plus trailing(0 0), we just allocate 2 more bytes and
keep the parameter of length unchanged.

         nLength = pApp->JS_appResponse(swQuestion, swTitle, swDefault, swLabel, bPassWord, pBuff, nLength);
-        if(nLength<=0)
+        if (nLength <= 0 || nLength > sizeof(pBuff))

[jun_fang, 2014/07/29 04:11:30]

Sizeof(pBuff) is 4 which represents bytes of a pointer. It may not work
correctly here. You may use strlen(pBuff).

[Tom Sepez, 2014/07/29 16:55:57]

Good catch.  Looks like this has been wrong for years!  Strlen might be bad
depending upon the way we handle nulls above (we shouldn't really need them if
we have explicit lengths, right?). And walking a string to find its length when
we were just told it by the method seems wasteful.

[jun_fang, 2014/07/29 17:31:47]

Strlen depends upon the last char '\0'. That's why I suggest to initialize the
buffer with '\0' first. Even walking a string, we don't have a rule to judge
which char is the last one.

         {
+                vRet.SetNull();
                 delete[] pBuff;
-                vRet.SetNull();
                 return FALSE;
         }
-        else
-        {
-                nLength = nLength > sizeof(pBuff) ? sizeof(pBuff) : nLength;
-        vRet = swResponse = CFX_WideString::FromUTF16LE((unsigned short*)pBuff, nLength / 2);
-        }
+
+        vRet = CFX_WideString::FromUTF16LE((unsigned short*)pBuff, nLength / 2);
         delete[] pBuff;
-
         return TRUE;
 }
 
 FX_BOOL app::media(OBJ_PROP_PARAMS)
 {
         return FALSE;
 }
 
 FX_BOOL app::execDialog(OBJ_METHOD_PARAMS)
 {
         return TRUE;
 }