Fix for UMR in CXML_Parser::GetCharRef.

Fix for UMR in CXML_Parser::GetCharRef.

BUG=387822
R=jun_fang@foxitsoftware.com

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

[Robert Sesek, 2014-07-08 16:34:28]

Ping? Please review this CL.

[jun_fang, 2014-07-08 17:37:11]

https://codereview.chromium.org/367383002/diff/60001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/367383002/diff/60001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2427: if (m_Pos >=
m_FileLen) {
Type
m_Pos:     FX_FILESIZE -> FX_INT32->int
m_FileLen: FX_FILESIZE -> FX_INT32->int
at line 2410: FX_DWORD len = 0; FX_DWORD->unsigned int
at line 2426: m_Pos += len;

len's scope is larger than m_Pos and m_FileLen.

Suggestion:
change FX_DWORD to FX_FILESIZE at line 2410.

[Robert Sesek, 2014-07-08 18:35:57]

Thank you for the review.

https://codereview.chromium.org/367383002/diff/60001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/367383002/diff/60001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2427: if (m_Pos >=
m_FileLen) {
On 2014/07/08 17:37:11, jun_fang wrote:
> Type
> m_Pos:     FX_FILESIZE -> FX_INT32->int
> m_FileLen: FX_FILESIZE -> FX_INT32->int
> at line 2410: FX_DWORD len = 0; FX_DWORD->unsigned int
> at line 2426: m_Pos += len;
> 
> len's scope is larger than m_Pos and m_FileLen.
> 
> Suggestion:
> change FX_DWORD to FX_FILESIZE at line 2410. 

Done, and Chris Palmer also suggested using CheckedNumeric because for a
sufficiently large value of |len|, it could still overflow.

[jun_fang, 2014-07-08 18:49:22]

On 2014/07/08 18:35:57, rsesek wrote:
> Thank you for the review.
> 
>
https://codereview.chromium.org/367383002/diff/60001/core/src/fpdfapi/fpdf_pa...
> File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):
> 
>
https://codereview.chromium.org/367383002/diff/60001/core/src/fpdfapi/fpdf_pa...
> core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2427: if (m_Pos >=
> m_FileLen) {
> On 2014/07/08 17:37:11, jun_fang wrote:
> > Type
> > m_Pos:     FX_FILESIZE -> FX_INT32->int
> > m_FileLen: FX_FILESIZE -> FX_INT32->int
> > at line 2410: FX_DWORD len = 0; FX_DWORD->unsigned int
> > at line 2426: m_Pos += len;
> > 
> > len's scope is larger than m_Pos and m_FileLen.
> > 
> > Suggestion:
> > change FX_DWORD to FX_FILESIZE at line 2410. 
> 
> Done, and Chris Palmer also suggested using CheckedNumeric because for a
> sufficiently large value of |len|, it could still overflow.

LGTM

[jun_fang, 2014-07-08 19:19:02]

On 2014/07/08 18:49:22, jun_fang wrote:
> On 2014/07/08 18:35:57, rsesek wrote:
> > Thank you for the review.
> > 
> >
>
https://codereview.chromium.org/367383002/diff/60001/core/src/fpdfapi/fpdf_pa...
> > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):
> > 
> >
>
https://codereview.chromium.org/367383002/diff/60001/core/src/fpdfapi/fpdf_pa...
> > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2427: if (m_Pos >=
> > m_FileLen) {
> > On 2014/07/08 17:37:11, jun_fang wrote:
> > > Type
> > > m_Pos:     FX_FILESIZE -> FX_INT32->int
> > > m_FileLen: FX_FILESIZE -> FX_INT32->int
> > > at line 2410: FX_DWORD len = 0; FX_DWORD->unsigned int
> > > at line 2426: m_Pos += len;
> > > 
> > > len's scope is larger than m_Pos and m_FileLen.
> > > 
> > > Suggestion:
> > > change FX_DWORD to FX_FILESIZE at line 2410. 
> > 
> > Done, and Chris Palmer also suggested using CheckedNumeric because for a
> > sufficiently large value of |len|, it could still overflow.
> 
> LGTM

Hi rsesek,

This patch can remove the redundant code and avoid potential integer overflow.
However it may not fix bug 387822. We can find the root cause of bug 387822 is
that memory isn't initialized after being allocated from the following stack
dump:


Uninitialized value was created by a heap allocation
    #0 0x7f222adfcc53 in __interceptor_malloc
src/third_party/llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:812
    #1 0x7f222b484296 in FXMEM_CreateMemoryMgr
src/third_party/pdfium/core/src/fxcrt/fx_basic_memmgr_mini.cpp:77
    #2 0x7f222ae50c59 in FPDF_InitLibrary
src/third_party/pdfium/fpdfsdk/src/fpdfview.cpp:165
    #3 0x7f222ae4c99e in main src/third_party/pdfium/samples/pdfium_test.cc:274
    #4 0x7f2229875de4 in __libc_start_main
/build/buildd/eglibc-2.17/csu/libc-start.c:260

[Robert Sesek, 2014-07-08 19:45:38]

On 2014/07/08 19:19:02, jun_fang wrote:
> On 2014/07/08 18:49:22, jun_fang wrote:
> > On 2014/07/08 18:35:57, rsesek wrote:
> > > Thank you for the review.
> > > 
> > >
> >
>
https://codereview.chromium.org/367383002/diff/60001/core/src/fpdfapi/fpdf_pa...
> > > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/367383002/diff/60001/core/src/fpdfapi/fpdf_pa...
> > > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2427: if (m_Pos >=
> > > m_FileLen) {
> > > On 2014/07/08 17:37:11, jun_fang wrote:
> > > > Type
> > > > m_Pos:     FX_FILESIZE -> FX_INT32->int
> > > > m_FileLen: FX_FILESIZE -> FX_INT32->int
> > > > at line 2410: FX_DWORD len = 0; FX_DWORD->unsigned int
> > > > at line 2426: m_Pos += len;
> > > > 
> > > > len's scope is larger than m_Pos and m_FileLen.
> > > > 
> > > > Suggestion:
> > > > change FX_DWORD to FX_FILESIZE at line 2410. 
> > > 
> > > Done, and Chris Palmer also suggested using CheckedNumeric because for a
> > > sufficiently large value of |len|, it could still overflow.
> > 
> > LGTM
> 
> Hi rsesek,
> 
> This patch can remove the redundant code and avoid potential integer overflow.
> However it may not fix bug 387822. We can find the root cause of bug 387822 is
> that memory isn't initialized after being allocated from the following stack
> dump:
> 
> 
> Uninitialized value was created by a heap allocation
>     #0 0x7f222adfcc53 in __interceptor_malloc
> src/third_party/llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:812
>     #1 0x7f222b484296 in FXMEM_CreateMemoryMgr
> src/third_party/pdfium/core/src/fxcrt/fx_basic_memmgr_mini.cpp:77
>     #2 0x7f222ae50c59 in FPDF_InitLibrary
> src/third_party/pdfium/fpdfsdk/src/fpdfview.cpp:165
>     #3 0x7f222ae4c99e in main
src/third_party/pdfium/samples/pdfium_test.cc:274
>     #4 0x7f2229875de4 in __libc_start_main
> /build/buildd/eglibc-2.17/csu/libc-start.c:260

I downloaded the test case from Clusterfuzz and verified with a local ASAN build
that this does indeed fix the issue. I believe that stack fragment is just
reporting where the memory was initially malloc()ed.

[palmer, 2014-07-08 21:02:08]

Committed patchset #5 manually as re7ee98e (presubmit successful).