Fix the potential integer overflow from 'offset+size' in extension.h and fpdfview.cpp

Fix the potential integer overflow from 'offset+size' in extension.h and fpdfview.cpp

BUG=397258
R=tsepez@chromium.org

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

[jun_fang, 2014-07-24 22:07:31]

Hi Chris, please start to review it. Thanks!

[palmer, 2014-07-25 00:40:14]

Tom, can I defer to you to see this one through to completion? I am trying to
delegate some pdfium code reviews. :) Thanks.

https://codereview.chromium.org/419063002/diff/40001/core/src/fxcrt/extension.h
File core/src/fxcrt/extension.h (right):

https://codereview.chromium.org/419063002/diff/40001/core/src/fxcrt/extension...
core/src/fxcrt/extension.h:71: base::CheckedNumeric<FX_FILESIZE> pos = size;
Putting something like this:

    typedef base::CheckedNumeric<FX_FILESIZE> FileSizeT;

at the top of the file might make the code look a little cleaner. (And then you
could also use the typedef in other files that #include this one.)

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

https://codereview.chromium.org/419063002/diff/40001/fpdfsdk/src/fpdfview.cpp...
fpdfsdk/src/fpdfview.cpp:302: if (!newPos.IsValid() || newPos.ValueOrDie() >=
(FX_DWORD)m_size) return FALSE;
Hmm, I wonder if this cast to FX_DWORD makes sense. On some platforms, it is
potentially truncating? I.e. on some platforms FX_FILESIZE is an off_t, which
might be 64 bits even on platforms where size_t and FX_DWORD are 32 bits.

I think it might be better to declare

    base::CheckedNumeric<FX_FILESIZE> newPos = size;

and then not cast |m_size| to FX_DWORD in the comparison expression.

[jun_fang, 2014-07-25 01:04:00]

https://codereview.chromium.org/419063002/diff/40001/core/src/fxcrt/extension.h
File core/src/fxcrt/extension.h (right):

https://codereview.chromium.org/419063002/diff/40001/core/src/fxcrt/extension...
core/src/fxcrt/extension.h:71: base::CheckedNumeric<FX_FILESIZE> pos = size;
On 2014/07/25 00:40:14, Chromium Palmer wrote:
> Putting something like this:
> 
>     typedef base::CheckedNumeric<FX_FILESIZE> FileSizeT;
> 
> at the top of the file might make the code look a little cleaner. (And then
you
> could also use the typedef in other files that #include this one.)

Good idea.

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

https://codereview.chromium.org/419063002/diff/40001/fpdfsdk/src/fpdfview.cpp...
fpdfsdk/src/fpdfview.cpp:302: if (!newPos.IsValid() || newPos.ValueOrDie() >=
(FX_DWORD)m_size) return FALSE;
On 2014/07/25 00:40:14, Chromium Palmer wrote:
> Hmm, I wonder if this cast to FX_DWORD makes sense. On some platforms, it is
> potentially truncating? I.e. on some platforms FX_FILESIZE is an off_t, which
> might be 64 bits even on platforms where size_t and FX_DWORD are 32 bits.
> 
> I think it might be better to declare
> 
>     base::CheckedNumeric<FX_FILESIZE> newPos = size;
> 
> and then not cast |m_size| to FX_DWORD in the comparison expression.

However, FX_FILESIZE is defined as int on some platforms. It causes potential
truncation if  size_t or FX_DWORD casts to FX_FILESIZE.

[palmer, 2014-07-25 18:26:55]

> > I think it might be better to declare
> > 
> >     base::CheckedNumeric<FX_FILESIZE> newPos = size;
> > 
> > and then not cast |m_size| to FX_DWORD in the comparison expression.
> 
> However, FX_FILESIZE is defined as int on some platforms. It causes potential
> truncation if  size_t or FX_DWORD casts to FX_FILESIZE.

Ouch.

For that kind of mess, you can (and should) use base::checked_cast<foo>(bar) in
safe_conversions.h.

Going forward, we should seek to reduce these custom typedefs and stick to the
standard stdint.h types (off_t, size_t, int64_t, et c.).

[jun_fang, 2014-07-29 16:59:28]

Hi Tom, can you review the last change? Thanks!

[Tom Sepez, 2014-07-30 18:50:25]

This is looking really good. LGTM with nits.

https://codereview.chromium.org/419063002/diff/100001/core/src/fxcrt/extension.h
File core/src/fxcrt/extension.h (right):

https://codereview.chromium.org/419063002/diff/100001/core/src/fxcrt/extensio...
core/src/fxcrt/extension.h:69: FX_SAFE_FILESIZE pos = size;
nit:  I would write this as
if (offset < 0 || size < 0) {
    return FALSE;
}
FX_SAFE_FILESIZE pos = size;
pos += offset;
if (!pos.IsVald() || pos.ValueOrDie() >= m_pFile->GetSize) {
    return FALSE;
}
just to avoid initializing the FX_SAFE_FILESIZE if we don't have to. I think
that reads a litte clearer.

https://codereview.chromium.org/419063002/diff/100001/core/src/fxcrt/extensio...
core/src/fxcrt/extension.h:193: if (!range.IsValid() || offset <= 0 || size <= 0
|| range.ValueOrDie() >= m_nCurSize) {
nit: again, I'd make the <= 0 checks first before initializing the
FX_SAFE_FILESIZE

https://codereview.chromium.org/419063002/diff/100001/core/src/fxcrt/extensio...
core/src/fxcrt/extension.h:303: if (!newPos.IsValid())
nit: braces here around single-line if to match this file's convention.

https://codereview.chromium.org/419063002/diff/100001/fpdfsdk/src/fpdfview.cpp
File fpdfsdk/src/fpdfview.cpp (right):

https://codereview.chromium.org/419063002/diff/100001/fpdfsdk/src/fpdfview.cp...
fpdfsdk/src/fpdfview.cpp:40: if (!newPos.IsValid() || newPos.ValueOrDie() >=
m_FileAccess.m_FileLen) return FALSE;
nit: newline and indent rather than single-line if?

https://codereview.chromium.org/419063002/diff/100001/fpdfsdk/src/fpdfview.cp...
fpdfsdk/src/fpdfview.cpp:50: if (!newPos.IsValid() || newPos.ValueOrDie() >=
m_FileAccess.m_FileLen) return FALSE;
nit:ditto

https://codereview.chromium.org/419063002/diff/100001/fpdfsdk/src/fpdfview.cp...
fpdfsdk/src/fpdfview.cpp:299: 
nit: ditto.

[jun_fang, 2014-07-30 20:26:55]

Please review it again. Thanks!

[Tom Sepez, 2014-07-30 20:29:27]

LGTM++.

[jun_fang, 2014-07-30 20:46:56]

Committed patchset #8 manually as r8dee6ca (presubmit successful).

[Tom Sepez, 2014-07-31 18:46:42]

https://codereview.chromium.org/419063002/diff/130001/core/include/fxcrt/fx_s...
File core/include/fxcrt/fx_system.h (right):

https://codereview.chromium.org/419063002/diff/130001/core/include/fxcrt/fx_s...
core/include/fxcrt/fx_system.h:282: typedef base::CheckedNumeric<size_t>        
   FX_SAFE_SIZET;
nit: Just to be pedantic, I'd have called this FX_SAFE_SIZE_T, just in case some
clown invents a type called sizet distinct from size_t, and tries to use a
FX_SAFE_SIZET with it ... :).