Fix integer issues leading to out of bounds access in fx_ge_text.cpp.

Fix integer issues leading to out of bounds access in fx_ge_text.cpp.

- Using |-skew| to get positive index, which doesn't work when skew is
  INT_MIN
- Incorrect logic when determining when to use |-skew| as an index.

R=tsepez@chromium.org,weili@chromium.org
BUG=chromium:601362
Committed: https://pdfium.googlesource.com/pdfium/+/b8627c9d13884d48943d8a7a5381eaf0bb2c08d9

[Oliver Chang, 2016-04-08 21:45:43]

Tom, Wei, for review.

Looks like this got introduced with recent refactoring/cleanups.

[Tom Sepez, 2016-04-08 21:48:40]

Can we add a test?

[Oliver Chang, 2016-04-08 23:41:52]

On 2016/04/08 21:48:40, Tom Sepez wrote:
> Can we add a test?

We have at least 2 testcases (one from reporter, and one from internal fuzzer),
but not sure if there are copyright issues with the original testcases they're
mutated from. I can ask the reporter if we can use the testcase they gave, but
in the meantime can we get the fix landed?

[Wei Li, 2016-04-11 16:40:37]

https://codereview.chromium.org/1875673004/diff/40001/core/fxge/ge/fx_ge_text...
File core/fxge/ge/fx_ge_text.cpp (right):

https://codereview.chromium.org/1875673004/diff/40001/core/fxge/ge/fx_ge_text...
core/fxge/ge/fx_ge_text.cpp:1574: static_cast<size_t>(-skew) <
ANGLESKEW_ARRAY_SIZE) {
/skew < 0/skew <= 0/

Also, casting to size_t makes -INT_MIN a very large unsigned value, so the
comparison with min() is not needed. In fact, even "skew<=0" could be skipped
too.

[Oliver Chang, 2016-04-11 16:51:42]

https://codereview.chromium.org/1875673004/diff/40001/core/fxge/ge/fx_ge_text...
File core/fxge/ge/fx_ge_text.cpp (right):

https://codereview.chromium.org/1875673004/diff/40001/core/fxge/ge/fx_ge_text...
core/fxge/ge/fx_ge_text.cpp:1574: static_cast<size_t>(-skew) <
ANGLESKEW_ARRAY_SIZE) {
On 2016/04/11 16:40:37, Wei Li wrote:
> /skew < 0/skew <= 0/

Done. 

> 
> Also, casting to size_t makes -INT_MIN a very large unsigned value, so the
> comparison with min() is not needed. In fact, even "skew<=0" could be skipped
> too.

The issue is that -INT_MIN is undefined, and I want to avoid invoking undefined
behaviour. It seems fine to me to keep the <= 0 check though to be on the safe
side, as it should be a pretty cheap check?

[Wei Li, 2016-04-11 17:00:47]

lgtm with comments below.

https://codereview.chromium.org/1875673004/diff/40001/core/fxge/ge/fx_ge_text...
File core/fxge/ge/fx_ge_text.cpp (right):

https://codereview.chromium.org/1875673004/diff/40001/core/fxge/ge/fx_ge_text...
core/fxge/ge/fx_ge_text.cpp:1842: if (skew < 0 && skew !=
std::numeric_limits<int>::min() &&
skew <= 0 here too.

Since you are here, can you pls change the comments in both places as well?
"skew should be non-positive so -skew can be used as the index. Also skew should
not be INT_MIN to avoid integer overflow." or sth like this. Thanks.

[Oliver Chang, 2016-04-11 17:06:47]

https://codereview.chromium.org/1875673004/diff/40001/core/fxge/ge/fx_ge_text...
File core/fxge/ge/fx_ge_text.cpp (right):

https://codereview.chromium.org/1875673004/diff/40001/core/fxge/ge/fx_ge_text...
core/fxge/ge/fx_ge_text.cpp:1842: if (skew < 0 && skew !=
std::numeric_limits<int>::min() &&
On 2016/04/11 17:00:47, Wei Li wrote:
> skew <= 0 here too.
> 
> Since you are here, can you pls change the comments in both places as well?
> "skew should be non-positive so -skew can be used as the index. Also skew
should
> not be INT_MIN to avoid integer overflow." or sth like this. Thanks.

Already made this <= 0 too. Added the comment

[Oliver Chang, 2016-04-11 19:55:00]

Tom, I've uploaded a test (hand mutated/minimised from a simple PDF I
generated). Please take a look.

[Tom Sepez, 2016-04-11 20:34:25]

lgtm

[Oliver Chang, 2016-04-11 20:35:22]

The CQ bit was checked by ochang@chromium.org

[Oliver Chang, 2016-04-11 20:35:22]

The patchset sent to the CQ was uploaded after l-g-t-m from weili@chromium.org
Link to the patchset: https://codereview.chromium.org/1875673004/#ps120001
(title: "fix naming")

[commit-bot: I haz the power, 2016-04-11 20:35:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1875673004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1875673004/120001

[commit-bot: I haz the power, 2016-04-11 20:47:46]

Description was changed from

==========
Fix integer issues leading to out of bounds access in fx_ge_text.cpp.

- Using |-skew| to get positive index, which doesn't work when skew is
  INT_MIN
- Incorrect logic when determining when to use |-skew| as an index.

R=tsepez@chromium.org,weili@chromium.org
BUG=chromium:601362
==========

to

==========
Fix integer issues leading to out of bounds access in fx_ge_text.cpp.

- Using |-skew| to get positive index, which doesn't work when skew is
  INT_MIN
- Incorrect logic when determining when to use |-skew| as an index.

R=tsepez@chromium.org,weili@chromium.org
BUG=chromium:601362

Committed:
https://pdfium.googlesource.com/pdfium/+/b8627c9d13884d48943d8a7a5381eaf0bb2c...
==========

[commit-bot: I haz the power, 2016-04-11 20:47:47]

Committed patchset #7 (id:120001) as
https://pdfium.googlesource.com/pdfium/+/b8627c9d13884d48943d8a7a5381eaf0bb2c...