Tidy up app::response().

fpdfsdk/src/javascript/app.cpp

Index: fpdfsdk/src/javascript/app.cpp
diff --git a/fpdfsdk/src/javascript/app.cpp b/fpdfsdk/src/javascript/app.cpp
index 3b92a992d9d0be2b46db2266d0e51847b0a34fdd..0500655055e6e4c3c1ea1a3119ad8214ab9acd88 100644
--- a/fpdfsdk/src/javascript/app.cpp
+++ b/fpdfsdk/src/javascript/app.cpp
@@ -1037,15 +1037,13 @@ FX_BOOL app::response(OBJ_METHOD_PARAMS)
 	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();
@@ -1104,19 +1102,15 @@ FX_BOOL app::response(OBJ_METHOD_PARAMS)
 	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.

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