Fix an out-of-boundary issue for wide string

Fix an out-of-boundary issue for wide string

BUG=381521
R=palmer@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/8daab31

[Bo Xu, 2014-07-10 16:26:12]

@Palmer, in https://code.google.com/p/chromium/issues/detail?id=381521#c6, you
mentioned about computing the |len| in the caller. I looked at the 5 callers, 3
of them needs to compute the length inside. Computing them outside seems
redundant (considering FPDF_WIDESTRING is not even a class but just a short*
type, otherwise could use a member function getLength()).

[palmer, 2014-07-10 19:06:47]

> @Palmer, in https://code.google.com/p/chromium/issues/detail?id=381521#c6, you
> mentioned about computing the |len| in the caller. I looked at the 5 callers,
3
> of them needs to compute the length inside. Computing them outside seems
> redundant (considering FPDF_WIDESTRING is not even a class but just a short*
> type, otherwise could use a member function getLength()).

The idea of the callee determining the length of a string (as in e.g. strcpy)
was a historical mistake, and we should not continue it. The core problem is
that the callee has to assume that the string does indeed end in a NUL byte, but
that assumption does not always hold. The caller, not the callee, must have (or,
should have; or, at least can have) the necessary information; the callee
sometimes cannot have that information and ideally should not have to make any
potentially-false assumptions about its input.

Additionally, the "length or magical -1" interface of the FromUTF16LE function
makes the function harder to reason about. Simple interfaces that always behave
the same way are easier to reason about.

Finally, it seems I was wrong in
https://code.google.com/p/chromium/issues/detail?id=381521#c6 — there are
currently 2 callers that make use of the default -1 value for the length
argument:

~/chromium/src/third_party/pdfium $ search -C FromUTF16LE -v
[*] ./fpdfsdk/src/fpdfdoc.cpp:39:   CFX_WideString wstr =
CFX_WideString::FromUTF16LE(title);
./fpdfsdk/src/fpdftext.cpp:165:        
textpageFind->FindFirst(CFX_WideString::FromUTF16LE(findwhat),flags,start_index);
./fpdfsdk/src/javascript/app.cpp:1117:          swResponse =
CFX_WideString::FromUTF16LE((unsigned short*)pBuff, nLength);
./core/src/fxcrt/fx_basic_buffer.cpp:374:    str =
CFX_WideString::FromUTF16LE((const unsigned short*)(FX_LPCSTR)encoded,
encoded.GetLength());
[*] ./core/src/fxge/win32/fx_win32_device.cpp:331:            CFX_WideString
wsName = CFX_WideString::FromUTF16LE((const unsigned
short*)VariantNames[i].m_pVariantName);

What is the 3rd one?

Anyway, (a) callee-must-assume is a bad idea; and (b) it's easy to fix a handful
of callers.

[palmer, 2014-07-10 19:10:55]

I think you should change the interface of FromUTF16LE and FromUTF8 (and
others?) so that the magic -1 no longer makes the callee assume the string is
0-terminated.

https://codereview.chromium.org/383563002/diff/1/fpdfsdk/src/javascript/app.cpp
File fpdfsdk/src/javascript/app.cpp (right):

https://codereview.chromium.org/383563002/diff/1/fpdfsdk/src/javascript/app.c...
fpdfsdk/src/javascript/app.cpp:1114: nLength = nLength>2048?2048:nLength;
Why 2048?

It looks like you are removing the code that ensured the string terminated with
a 0 character/short? Why?

https://codereview.chromium.org/383563002/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cpp
File fpdfsdk/src/jsapi/fxjs_v8.cpp (right):

https://codereview.chromium.org/383563002/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cp...
fpdfsdk/src/jsapi/fxjs_v8.cpp:737: return CFX_WideString::FromUTF8(*s,
s.length());
Ahh, I see, FromUTF8 has the same behavior. There are another 2 callers that
rely on the callee-must-assume behavior:

./fpdfsdk/src/javascript/Document.cpp:1073:                     CFX_WideString
wsKey  = CFX_WideString::FromUTF8(bsKey);

./core/src/fxcrt/fx_basic_util.cpp:276:    return
CFX_WideString::FromUTF8(rURI);

[Bo Xu, 2014-07-10 19:22:10]

On 2014/07/10 19:10:55, Chromium Palmer wrote:
> I think you should change the interface of FromUTF16LE and FromUTF8 (and
> others?) so that the magic -1 no longer makes the callee assume the string is
> 0-terminated.
> 
>
https://codereview.chromium.org/383563002/diff/1/fpdfsdk/src/javascript/app.cpp
> File fpdfsdk/src/javascript/app.cpp (right):
> 
>
https://codereview.chromium.org/383563002/diff/1/fpdfsdk/src/javascript/app.c...
> fpdfsdk/src/javascript/app.cpp:1114: nLength = nLength>2048?2048:nLength;
> Why 2048?
> 
> It looks like you are removing the code that ensured the string terminated
with
> a 0 character/short? Why?
> 
> https://codereview.chromium.org/383563002/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cpp
> File fpdfsdk/src/jsapi/fxjs_v8.cpp (right):
> 
>
https://codereview.chromium.org/383563002/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cp...
> fpdfsdk/src/jsapi/fxjs_v8.cpp:737: return CFX_WideString::FromUTF8(*s,
> s.length());
> Ahh, I see, FromUTF8 has the same behavior. There are another 2 callers that
> rely on the callee-must-assume behavior:
> 
> ./fpdfsdk/src/javascript/Document.cpp:1073:                     CFX_WideString
> wsKey  = CFX_WideString::FromUTF8(bsKey);
> 
> ./core/src/fxcrt/fx_basic_util.cpp:276:    return
> CFX_WideString::FromUTF8(rURI);

OK. I will make the changes for these similar function calls. How do you suggest
the caller to compute the length? Same way is it is in the callee?

[Bo Xu, 2014-07-10 19:22:16]

https://codereview.chromium.org/383563002/diff/1/fpdfsdk/src/javascript/app.cpp
File fpdfsdk/src/javascript/app.cpp (right):

https://codereview.chromium.org/383563002/diff/1/fpdfsdk/src/javascript/app.c...
fpdfsdk/src/javascript/app.cpp:1114: nLength = nLength>2048?2048:nLength;
On 2014/07/10 19:10:55, Chromium Palmer wrote:
> Why 2048?
> 
> It looks like you are removing the code that ensured the string terminated
with
> a 0 character/short? Why?

In line 1105, the pBuff is newed to be a length of 2048, so this is to keep the
length within range. I agree this is kinda not strict. Any suggestion to remedy
this?

[palmer, 2014-07-11 00:12:29]

https://codereview.chromium.org/383563002/diff/1/fpdfsdk/src/javascript/app.cpp
File fpdfsdk/src/javascript/app.cpp (right):

https://codereview.chromium.org/383563002/diff/1/fpdfsdk/src/javascript/app.c...
fpdfsdk/src/javascript/app.cpp:1114: nLength = nLength>2048?2048:nLength;
> In line 1105, the pBuff is newed to be a length of 2048, so this is to keep
the
> length within range. I agree this is kinda not strict. Any suggestion to
remedy
> this?

So, does it matter if we effectively truncate the string? If
pApp->JS_appResponse returns an nLength > than the # of bytes we allocated for
|pBuff|, it is telling us that we needed to have allocated a larger string. Some
applications would then delete[] pBuff, then re-allocate pBuff to the new
nLength, then re-invoke pApp->JS_appResponse.

But, if it is OK to set swResponse to a string that we know is not the full
result of pApp->JS_appResponse, then we should at least ensure that the string
is 0-terminated. Probably. Right?

How about this:

nLength = pApp->JS_appResponse(swQuestion, swTitle, swDefault, swLabel,
bPassWord, pBuff, nLength);

if (nLength <= 0) {
    vRet.SetNull();
    return FALSE;
} else {
    nLength = nLength > sizeof(pBuff) ? sizeof(pBuff) : nLength;
    pBuff[nLength - 1] = '\0';
    pBuff[nLength - 2] = '\0';
    vRet = swResponse = CFX_WideString::FromUTF16LE((unsigned short*)pBuff,
nLength / 2);
}

[Bo Xu, 2014-07-11 03:02:38]

https://codereview.chromium.org/383563002/diff/1/fpdfsdk/src/javascript/app.cpp
File fpdfsdk/src/javascript/app.cpp (right):

https://codereview.chromium.org/383563002/diff/1/fpdfsdk/src/javascript/app.c...
fpdfsdk/src/javascript/app.cpp:1114: nLength = nLength>2048?2048:nLength;
The SDK only supports 2048 byte, so here is a hard cut.

The code here is not that straightforward, the WideString is zeroed in
CFX_WideString::FromUTF16LE, in CFX_WideString::GetBuffer, in static
CFX_StringDataW* FX_AllocStringW.

So we don't have to zero out here. It's not that obvious though.

[Bo Xu, 2014-07-11 03:29:22]

I made some changes. Please take a look at Patch set 3.

[palmer, 2014-07-12 00:54:39]

Definitely better to use a library function than inlining your own wcslen/strlen
equivalent. If wcslen doesn't work, define your own, once, in some utility
function file.

https://codereview.chromium.org/383563002/diff/40001/core/src/fxcrt/fx_basic_...
File core/src/fxcrt/fx_basic_wstring.cpp (right):

https://codereview.chromium.org/383563002/diff/40001/core/src/fxcrt/fx_basic_...
core/src/fxcrt/fx_basic_wstring.cpp:401: if (!str) {
You could say:

    if (!str || 0 == len)

to avoid the (infinitesimal) cost of constructing and calling CFX_UTF8Decoder.

Then this function would match the behavior on line 413.

https://codereview.chromium.org/383563002/diff/40001/core/src/fxge/win32/fx_w...
File core/src/fxge/win32/fx_win32_device.cpp (right):

https://codereview.chromium.org/383563002/diff/40001/core/src/fxge/win32/fx_w...
core/src/fxge/win32/fx_win32_device.cpp:333: while (pName[len]) len++;
Use wcslen?

https://codereview.chromium.org/383563002/diff/40001/fpdfsdk/src/fpdfdoc.cpp
File fpdfsdk/src/fpdfdoc.cpp (right):

https://codereview.chromium.org/383563002/diff/40001/fpdfsdk/src/fpdfdoc.cpp#...
fpdfsdk/src/fpdfdoc.cpp:40: while (title[len]) len++;
wcslen?

https://codereview.chromium.org/383563002/diff/40001/fpdfsdk/src/fpdftext.cpp
File fpdfsdk/src/fpdftext.cpp (right):

https://codereview.chromium.org/383563002/diff/40001/fpdfsdk/src/fpdftext.cpp...
fpdfsdk/src/fpdftext.cpp:166: while (findwhat[len]) len++;
wcslen?

[Bo Xu, 2014-07-12 05:18:04]

wcslen cannot take const unsigned short*, so I wrote a function.

https://codereview.chromium.org/383563002/diff/40001/core/src/fxcrt/fx_basic_...
File core/src/fxcrt/fx_basic_wstring.cpp (right):

https://codereview.chromium.org/383563002/diff/40001/core/src/fxcrt/fx_basic_...
core/src/fxcrt/fx_basic_wstring.cpp:401: if (!str) {
On 2014/07/12 00:54:39, Chromium Palmer wrote:
> You could say:
> 
>     if (!str || 0 == len)
> 
> to avoid the (infinitesimal) cost of constructing and calling CFX_UTF8Decoder.
> 
> Then this function would match the behavior on line 413.

Done.

https://codereview.chromium.org/383563002/diff/40001/core/src/fxge/win32/fx_w...
File core/src/fxge/win32/fx_win32_device.cpp (right):

https://codereview.chromium.org/383563002/diff/40001/core/src/fxge/win32/fx_w...
core/src/fxge/win32/fx_win32_device.cpp:333: while (pName[len]) len++;
On 2014/07/12 00:54:39, Chromium Palmer wrote:
> Use wcslen?

Done.

https://codereview.chromium.org/383563002/diff/40001/fpdfsdk/src/fpdfdoc.cpp
File fpdfsdk/src/fpdfdoc.cpp (right):

https://codereview.chromium.org/383563002/diff/40001/fpdfsdk/src/fpdfdoc.cpp#...
fpdfsdk/src/fpdfdoc.cpp:40: while (title[len]) len++;
On 2014/07/12 00:54:39, Chromium Palmer wrote:
> wcslen?

Done.

https://codereview.chromium.org/383563002/diff/40001/fpdfsdk/src/fpdftext.cpp
File fpdfsdk/src/fpdftext.cpp (right):

https://codereview.chromium.org/383563002/diff/40001/fpdfsdk/src/fpdftext.cpp...
fpdfsdk/src/fpdftext.cpp:166: while (findwhat[len]) len++;
On 2014/07/12 00:54:39, Chromium Palmer wrote:
> wcslen?

Done.

[palmer, 2014-07-14 19:08:36]

lgtm

[Bo Xu, 2014-07-14 19:14:17]

Committed patchset #4 manually as r8daab31 (presubmit successful).