Fix potential UAF in ConcatInPlace.

Fix potential UAF in ConcatInPlace.

If ConcatCopy somehow gets a zero nNewlen, it returns early, without
allocating a new m_Data.  ConcatInPlace then frees the old one, leaving
m_Data dangling.

Also be concerned about the multiplication in the widestring version.
So use wmemcpy and let the library cope with it.

R=thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/7f3b99a6a78e524613337f42a99b5634c0ad05f8

[Tom Sepez, 2015-05-14 21:47:42]

tsepez@chromium.org changed reviewers:
+ thestig@chromium.org

[Tom Sepez, 2015-05-14 21:47:42]

Lei, this looks suspicious.

[Lei Zhang, 2015-05-14 22:07:55]

How do you feel about writing a unit test to trigger this?

https://codereview.chromium.org/1130763007/diff/1/core/src/fxcrt/fx_basic_wst...
File core/src/fxcrt/fx_basic_wstring.cpp (right):

https://codereview.chromium.org/1130763007/diff/1/core/src/fxcrt/fx_basic_wst...
core/src/fxcrt/fx_basic_wstring.cpp:261: if (pOldData) {
No need for the if () ? It's not in the bstring version, plus the only caller to
ConcatCopy() is ConcatInPlace(), which dereferences |m_pData| before calling
ConcatCopy().

[Tom Sepez, 2015-05-14 22:19:29]

On 2015/05/14 22:07:55, Lei Zhang wrote:
> How do you feel about writing a unit test to trigger this?
> 
Enthusiastic.  By this, do you mean the UAF or the bad splice?

[Lei Zhang, 2015-05-14 22:23:55]

On 2015/05/14 22:19:29, Tom Sepez wrote:
> On 2015/05/14 22:07:55, Lei Zhang wrote:
> > How do you feel about writing a unit test to trigger this?
> > 
> Enthusiastic.  By this, do you mean the UAF or the bad splice?

By bad splice, you mean the memory overlap? Both if possible.

BTW, typo in commit msg: "caclulate"

[Tom Sepez, 2015-05-14 23:03:19]

On 2015/05/14 22:23:55, Lei Zhang wrote:
> On 2015/05/14 22:19:29, Tom Sepez wrote:
> > On 2015/05/14 22:07:55, Lei Zhang wrote:
> > > How do you feel about writing a unit test to trigger this?
> > > 
> > Enthusiastic.  By this, do you mean the UAF or the bad splice?
> 
> By bad splice, you mean the memory overlap? Both if possible.
> 
> BTW, typo in commit msg: "caclulate"
Ah, the offset is right since the underlying pointer type is really wchar_t*,
not char* with casts.

[Tom Sepez, 2015-05-14 23:14:47]

OK, I added tests to cover the basic case, plus contrived one to hit the UAF.

[Lei Zhang, 2015-05-15 09:19:59]

lgtm

We don't need to mention wmemmove anymore in the CL description?

[Tom Sepez, 2015-05-15 15:39:24]

On 2015/05/15 09:19:59, Lei Zhang wrote:
> lgtm
> 
> We don't need to mention wmemmove anymore in the CL description?

Its now a wmemcpy.

[Tom Sepez, 2015-05-15 15:44:40]

Committed patchset #6 (id:100001) manually as
7f3b99a6a78e524613337f42a99b5634c0ad05f8 (presubmit successful).