Fix a bug when assign the generation number of indirect objects

Fix a bug when assign the generation number of indirect objects

BUG=408532
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/2d282243dbd1edd51d42e13f563903a1a76ce8f8

[Bo Xu, 2014-08-28 23:46:14]

bo_xu@foxitsoftware.com changed reviewers:
+ jun_fang@foxitsoftware.com, tsepez@chromium.org

[Bo Xu, 2014-08-28 23:46:14]

Hi Tom, please review.

[Bo Xu, 2014-08-29 03:09:27]

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

https://codereview.chromium.org/524443002/diff/60001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1396: if(pObj) {
Here when objnum >1, real_objnum must equal objnum here, so no need to check
!objnum.

[Tom Sepez, 2014-08-29 17:28:01]

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

https://codereview.chromium.org/524443002/diff/60001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1384: FX_DWORD gennum =
FXSYS_atoi(word);
nit: I'd call this real_gennum for parallelism with the real_objnum derived from
the parser above.  I found it confusing that objnum was a paramenter,
real_objnum was returned from the parser, but gennum wasn't a paramenter but was
returned from the parser, eh?

If you wanted to be really clear, I might get rid of the real_ prefix (what does
that mean, really) and call it parser_objnum or objnum_from_parser.

Your call on what to do here.  Thanks for listening.

https://codereview.chromium.org/524443002/diff/60001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1396: if(pObj) {
yes, I think you want this block to read

if (pObj) {
    if (!objnum)
        pObj->m_ObjNum = real_objnum;
    pObj->m_GenNum = gennum

[Tom Sepez, 2014-08-29 20:17:19]

(or perhaps parsed_objnum and parsed_gennum).

[Bo Xu, 2014-09-19 21:48:19]

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

https://codereview.chromium.org/524443002/diff/60001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1384: FX_DWORD gennum =
FXSYS_atoi(word);
On 2014/08/29 17:28:01, Tom Sepez wrote:
> nit: I'd call this real_gennum for parallelism with the real_objnum derived
from
> the parser above.  I found it confusing that objnum was a paramenter,
> real_objnum was returned from the parser, but gennum wasn't a paramenter but
was
> returned from the parser, eh?
> 
> If you wanted to be really clear, I might get rid of the real_ prefix (what
does
> that mean, really) and call it parser_objnum or objnum_from_parser.
> 
> Your call on what to do here.  Thanks for listening.

I feel parser_objnum is better.

https://codereview.chromium.org/524443002/diff/60001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1396: if(pObj) {
On 2014/08/29 17:28:01, Tom Sepez wrote:
> yes, I think you want this block to read
> 
> if (pObj) {
>     if (!objnum)
>         pObj->m_ObjNum = real_objnum;
>     pObj->m_GenNum = gennum

Done.

https://codereview.chromium.org/524443002/diff/80001/core/src/fxcrt/fx_basic_...
File core/src/fxcrt/fx_basic_gcc.cpp (right):

https://codereview.chromium.org/524443002/diff/80001/core/src/fxcrt/fx_basic_...
core/src/fxcrt/fx_basic_gcc.cpp:27: break;
Now check when the string is too long and overflow the integer.

[Tom Sepez, 2014-09-19 22:15:39]

https://codereview.chromium.org/524443002/diff/80001/core/src/fxcrt/fx_basic_...
File core/src/fxcrt/fx_basic_gcc.cpp (right):

https://codereview.chromium.org/524443002/diff/80001/core/src/fxcrt/fx_basic_...
core/src/fxcrt/fx_basic_gcc.cpp:27: break;
On 2014/09/19 21:48:19, Bo Xu wrote:
> Now check when the string is too long and overflow the integer.
This can't work, either T is a signed type in which case overflow is undefined,
or T is unsigned in which case overflow is defined but the value will never be
less than 0.

Use the safe math types here if you are worried.

[Bo Xu, 2014-09-19 22:28:15]

https://codereview.chromium.org/524443002/diff/80001/core/src/fxcrt/fx_basic_...
File core/src/fxcrt/fx_basic_gcc.cpp (right):

https://codereview.chromium.org/524443002/diff/80001/core/src/fxcrt/fx_basic_...
core/src/fxcrt/fx_basic_gcc.cpp:27: break;
On 2014/09/19 22:15:38, Tom Sepez wrote:
> On 2014/09/19 21:48:19, Bo Xu wrote:
> > Now check when the string is too long and overflow the integer.
> This can't work, either T is a signed type in which case overflow is
undefined,
> or T is unsigned in which case overflow is defined but the value will never be
> less than 0.
> 
> Use the safe math types here if you are worried.

Done.

[Tom Sepez, 2014-09-19 22:31:47]

Alternatively make a comparison of num  to (std::numeric_limits<T>::max() - 9) /
10 *before* the multiply and add.

[Tom Sepez, 2014-09-19 22:35:07]

https://codereview.chromium.org/524443002/diff/100001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_gcc.cpp (right):

https://codereview.chromium.org/524443002/diff/100001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc.cpp:19: base::CheckedNumeric<T> num = 0;
num doesn't need to be checked.

https://codereview.chromium.org/524443002/diff/100001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc.cpp:20: base::CheckedNumeric<T> num_new = num;
maybe this moves to right before line 25, and its called safe_num instead of new
num.

https://codereview.chromium.org/524443002/diff/100001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc.cpp:30: num = num_new;
num = new_num.ValueOrDie();

[Bo Xu, 2014-09-19 22:40:23]

On 2014/09/19 22:31:47, Tom Sepez wrote:
> Alternatively make a comparison of num  to (std::numeric_limits<T>::max() - 9)
/
> 10 *before* the multiply and add.

I kinda prefer this alternative approach. Does patch 7# looks good?

[Tom Sepez, 2014-09-19 22:43:01]

lgtm

[Tom Sepez, 2014-09-19 22:47:02]

(provided it compiles - you might want to #include <limits> explicitly).

[Bo Xu, 2014-09-19 22:55:40]

On 2014/09/19 22:47:02, Tom Sepez wrote:
> (provided it compiles - you might want to #include <limits> explicitly).

Ok, I added it.

[Tom Sepez, 2014-09-19 22:56:31]

lgtm

[Bo Xu, 2014-09-19 22:58:49]

Committed patchset #8 (id:140001) manually as 2d28224 (presubmit successful).