Remove custom memory manager

Remove custom memory manager

BUG=
R=palmer@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/3522876

[Bo Xu, 2014-07-04 18:55:23]

We have replaced the custom memory manager with system memory manager. Please
review.

[palmer, 2014-07-07 18:54:09]

https://codereview.chromium.org/372473003/diff/1/core/include/fxcrt/fx_memory.h
File core/include/fxcrt/fx_memory.h (right):

https://codereview.chromium.org/372473003/diff/1/core/include/fxcrt/fx_memory...
core/include/fxcrt/fx_memory.h:15: #define FX_Alloc(type,
size)						(type*)malloc(sizeof(type) * (size))
This and line 16 suffer from integer overflow risk.

[palmer, 2014-07-07 19:17:54]

Thank you for doing this!

There are a few places where you need to handle integer overflow, most often by
simply calling through to calloc. Other places may need CheckedNumeric.

https://codereview.chromium.org/372473003/diff/1/core/include/fxcrt/fx_basic.h
File core/include/fxcrt/fx_basic.h (right):

https://codereview.chromium.org/372473003/diff/1/core/include/fxcrt/fx_basic....
core/include/fxcrt/fx_basic.h:1470: list.data = FX_Alloc(FX_BYTE, temp_count *
unit);
Potential integer overflow here. Especially since you memset the memory to 0 on
line 1474, it is safer and faster to just use calloc.

(You might resolve the integer overflow in the definition of FX_Alloc by just
making it be a call to calloc.)

(Note that calloc has the potential to (a) check for overflow and (b) be faster
than malloc + memset.)

https://codereview.chromium.org/372473003/diff/1/core/src/fxcodec/codec/fx_co...
File core/src/fxcodec/codec/fx_codec.cpp (right):

https://codereview.chromium.org/372473003/diff/1/core/src/fxcodec/codec/fx_co...
core/src/fxcodec/codec/fx_codec.cpp:122: m_pDataCache =
(CCodec_ImageDataCache*)FX_AllocNL(FX_BYTE, sizeof(CCodec_ImageDataCache) +
m_Pitch * m_OutputHeight);
Potential integer overflow here.

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_arra...
File core/src/fxcrt/fx_basic_array.cpp (right):

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_arra...
core/src/fxcrt/fx_basic_array.cpp:44: m_pData = FX_Alloc(FX_BYTE, nNewSize *
m_nUnitSize);
Potential integer overflow.

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_arra...
core/src/fxcrt/fx_basic_array.cpp:49: FXSYS_memset32(m_pData, 0, nNewSize *
m_nUnitSize);
Integer overflow here and on line 53. You probably want calloc.

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_arra...
core/src/fxcrt/fx_basic_array.cpp:68: FX_LPBYTE pNewData = FX_Realloc(FX_BYTE,
m_pData, nNewMax * m_nUnitSize);
Potential integer overflow.

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_arra...
core/src/fxcrt/fx_basic_array.cpp:72: FXSYS_memset32(pNewData + m_nSize *
m_nUnitSize, 0, (nNewMax - m_nSize) * m_nUnitSize);
Potential integer overflow. Another reason to stick to calloc.

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_arra...
core/src/fxcrt/fx_basic_array.cpp:85: FXSYS_memcpy32(m_pData + nOldSize *
m_nUnitSize, src.m_pData, src.m_nSize * m_nUnitSize);
Not sure if all this arithmetic is safe against overflow.

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_plex...
File core/src/fxcrt/fx_basic_plex.cpp (right):

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_plex...
core/src/fxcrt/fx_basic_plex.cpp:11: CFX_Plex* p = (CFX_Plex*)FX_Alloc(FX_BYTE,
sizeof(CFX_Plex) + nMax * cbElement);
Potential integer overflow here.

[Bo Xu, 2014-07-08 02:54:29]

Looks like most of the issues here are related to over flow in FX_Alloc. We
could change this it to calloc but only a small fraction of the callers need the
memory to be set to zero. And this is called tons of times, would calloc be a
performance issue here?

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

https://codereview.chromium.org/372473003/diff/1/core/include/fxcrt/fx_memory.h
File core/include/fxcrt/fx_memory.h (right):

https://codereview.chromium.org/372473003/diff/1/core/include/fxcrt/fx_memory...
core/include/fxcrt/fx_memory.h:15: #define FX_Alloc(type,
size)						(type*)malloc(sizeof(type) * (size))
Do you suggest to replace here with a function call using numerics memory? A
function call may increase the overall load of the entire program?

On 2014/07/07 18:54:09, Chromium Palmer wrote:
> This and line 16 suffer from integer overflow risk.

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_arra...
File core/src/fxcrt/fx_basic_array.cpp (right):

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_arra...
core/src/fxcrt/fx_basic_array.cpp:49: FXSYS_memset32(m_pData, 0, nNewSize *
m_nUnitSize);
Here should e handled by the check in line 26

On 2014/07/07 19:17:53, Chromium Palmer wrote:
> Integer overflow here and on line 53. You probably want calloc.

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_arra...
core/src/fxcrt/fx_basic_array.cpp:85: FXSYS_memcpy32(m_pData + nOldSize *
m_nUnitSize, src.m_pData, src.m_nSize * m_nUnitSize);
The function SetSize does the overflow check, so here should be ok.

On 2014/07/07 19:17:53, Chromium Palmer wrote:
> Not sure if all this arithmetic is safe against overflow.

[Bo Xu, 2014-07-08 19:31:25]

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_arra...
File core/src/fxcrt/fx_basic_array.cpp (right):

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_arra...
core/src/fxcrt/fx_basic_array.cpp:72: FXSYS_memset32(pNewData + m_nSize *
m_nUnitSize, 0, (nNewMax - m_nSize) * m_nUnitSize);
Here the integer overflow has been checked by line 26.

On 2014/07/07 19:17:53, Chromium Palmer wrote:
> Potential integer overflow. Another reason to stick to calloc.

[palmer, 2014-07-08 22:13:13]

> Looks like most of the issues here are related to over flow in FX_Alloc. We
> could change this it to calloc but only a small fraction of the callers need
the
> memory to be set to zero. And this is called tons of times, would calloc be a
> performance issue here?

That's an empirical question. :) Try implementing FX_Alloc as an
integer-overflow-checked malloc, and then again as a call through to calloc, and
see what a benchmark shows.

[palmer, 2014-07-08 22:24:13]

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_arra...
File core/src/fxcrt/fx_basic_array.cpp (right):

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_arra...
core/src/fxcrt/fx_basic_array.cpp:72: FXSYS_memset32(pNewData + m_nSize *
m_nUnitSize, 0, (nNewMax - m_nSize) * m_nUnitSize);
> Here the integer overflow has been checked by line 26.

I am not sure that is a sufficient check (as explained in the next comment).

https://codereview.chromium.org/372473003/diff/1/core/src/fxcrt/fx_basic_arra...
core/src/fxcrt/fx_basic_array.cpp:85: FXSYS_memcpy32(m_pData + nOldSize *
m_nUnitSize, src.m_pData, src.m_nSize * m_nUnitSize);
> The function SetSize does the overflow check, so here should be ok.

I'm not sure. The expression "m_nSize + src.m_nSize" on line 82 could have
overflowed, for example.

And I'm not entirely sure about the implementation of SetSize. For example:

 45         m_pData = FX_Allocator_Alloc(m_pAllocator, FX_BYTE, nNewSize *
m_nUnitSize);

but |nNewSize| is only somewhat constrained:

 27     if (nNewSize < 0 || nNewSize > (1 << 28) / m_nUnitSize) {

and |m_nUnitSize| is not constrained at all.

[palmer, 2014-07-08 22:24:48]

I propose we get this patch in, and then deal with any remaining overflow
concerns in a later patch. LGTM.

[Bo Xu, 2014-07-08 22:30:52]

Committed patchset #2 manually as r3522876 (presubmit successful).

[Bo Xu, 2014-07-08 22:32:15]

On 2014/07/08 22:24:48, Chromium Palmer wrote:
> I propose we get this patch in, and then deal with any remaining overflow
> concerns in a later patch. LGTM.

Patch landed.