Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
509993003:
Restrict index not be greater than row_size in TIFF_PredictLine (Closed)
|
Created:
6 years, 3 months ago by Bo Xu Modified:
5 years, 9 months ago Base URL:
https://pdfium.googlesource.com/pdfium.git@master Visibility:
Public. |
DescriptionRestrict index not be greater than row_size in TIFF_PredictLine
There is not strict way to limit invalid value of |Colors| from dictionary. We can make sure |index| does not go out of boundary of row_size.
BUG=407614
R=tsepez@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/fdcc874
Patch Set 1 #
Total comments: 9
Patch Set 2 : #Patch Set 3 : Remove index_pre and col_pre #Messages
Total messages: 15 (0 generated)
bo_xu@foxitsoftware.com changed reviewers: + jun_fang@foxitsoftware.com, tsepez@chromium.org
On 2014/08/27 21:18:28, Bo Xu wrote: > mailto:bo_xu@foxitsoftware.com changed reviewers: > + mailto:jun_fang@foxitsoftware.com, mailto:tsepez@chromium.org Tom, please review this one.
https://codereview.chromium.org/509993003/diff/1/core/src/fxcodec/codec/fx_co... File core/src/fxcodec/codec/fx_codec_flate.cpp (right): https://codereview.chromium.org/509993003/diff/1/core/src/fxcodec/codec/fx_co... core/src/fxcodec/codec/fx_codec_flate.cpp:531: if (index > row_size) { Don't you want >= ?
https://codereview.chromium.org/509993003/diff/1/core/src/fxcodec/codec/fx_co... File core/src/fxcodec/codec/fx_codec_flate.cpp (right): https://codereview.chromium.org/509993003/diff/1/core/src/fxcodec/codec/fx_co... core/src/fxcodec/codec/fx_codec_flate.cpp:527: int row_bits = BitsPerComponent * Colors * Columns; nit: the multiply by 1 seems unnecessary. https://codereview.chromium.org/509993003/diff/1/core/src/fxcodec/codec/fx_co... core/src/fxcodec/codec/fx_codec_flate.cpp:531: if (index > row_size) { Maybe we prune row_bits outside the loop to avoid having to test each time. https://codereview.chromium.org/509993003/diff/1/core/src/fxcodec/codec/fx_co... core/src/fxcodec/codec/fx_codec_flate.cpp:534: int index_pre = (col == 0) ? (index - 1) : index; nit: maybe these should move outsize the loop, start at (0, 0) and get assigned to the current col, row at the bottom of the loop?
https://codereview.chromium.org/509993003/diff/1/core/src/fxcodec/codec/fx_co... File core/src/fxcodec/codec/fx_codec_flate.cpp (right): https://codereview.chromium.org/509993003/diff/1/core/src/fxcodec/codec/fx_co... core/src/fxcodec/codec/fx_codec_flate.cpp:535: int col_pre = (col == 0) ? 8 : col; 8 seems like a weird value to assign to a variable thats normally the resuult of i % 8 and should be in the 0..7 https://codereview.chromium.org/509993003/diff/1/core/src/fxcodec/codec/fx_co... core/src/fxcodec/codec/fx_codec_flate.cpp:536: if( ((dest_buf[index] >> (7 - col)) & 1) ^ ((dest_buf[index_pre] >> (8 - col_pre)) & 1) ) { This would need to change if you followed the above comment.
https://codereview.chromium.org/509993003/diff/1/core/src/fxcodec/codec/fx_co... File core/src/fxcodec/codec/fx_codec_flate.cpp (right): https://codereview.chromium.org/509993003/diff/1/core/src/fxcodec/codec/fx_co... core/src/fxcodec/codec/fx_codec_flate.cpp:527: int row_bits = BitsPerComponent * Colors * Columns; On 2014/08/27 21:25:45, Tom Sepez wrote: > nit: the multiply by 1 seems unnecessary. Maybe we should keep it here to make the meaning more straightforward? As |BitsPerComponent * Colors * Columns| is usually a binding group in elsewhere. https://codereview.chromium.org/509993003/diff/1/core/src/fxcodec/codec/fx_co... core/src/fxcodec/codec/fx_codec_flate.cpp:531: if (index > row_size) { On 2014/08/27 21:25:45, Tom Sepez wrote: > Maybe we prune row_bits outside the loop to avoid having to test each time. Good idea. https://codereview.chromium.org/509993003/diff/1/core/src/fxcodec/codec/fx_co... core/src/fxcodec/codec/fx_codec_flate.cpp:534: int index_pre = (col == 0) ? (index - 1) : index; On 2014/08/27 21:25:45, Tom Sepez wrote: > nit: maybe these should move outsize the loop, start at (0, 0) and get assigned > to the current col, row at the bottom of the loop? Done.
Good. LGTM. Thanks.
On 2014/08/27 22:17:56, Tom Sepez wrote: > Good. LGTM. Thanks. Optionally, if you want to be really picky, we don't need the previous index/col pair, just the previous bit value. That might be cleaner. Up to you.
On 2014/08/27 22:23:48, Tom Sepez wrote: > On 2014/08/27 22:17:56, Tom Sepez wrote: > > Good. LGTM. Thanks. > > Optionally, if you want to be really picky, we don't need the previous index/col > pair, just the previous bit value. That might be cleaner. Up to you. In that way, meaning we have to re-compute (i-1)/8 and (i-1)%8 in each loop? Maybe that's a bit slower?
On 2014/08/27 22:32:39, Bo Xu wrote: > On 2014/08/27 22:23:48, Tom Sepez wrote: > > On 2014/08/27 22:17:56, Tom Sepez wrote: > > > Good. LGTM. Thanks. > > > > Optionally, if you want to be really picky, we don't need the previous > index/col > > pair, just the previous bit value. That might be cleaner. Up to you. > > In that way, meaning we have to re-compute (i-1)/8 and (i-1)%8 in each loop? > Maybe that's a bit slower? Let's not worry about it. I think this is clearer.
On 2014/08/27 22:40:23, Tom Sepez wrote: > On 2014/08/27 22:32:39, Bo Xu wrote: > > On 2014/08/27 22:23:48, Tom Sepez wrote: > > > On 2014/08/27 22:17:56, Tom Sepez wrote: > > > > Good. LGTM. Thanks. > > > > > > Optionally, if you want to be really picky, we don't need the previous > > index/col > > > pair, just the previous bit value. That might be cleaner. Up to you. > > > > In that way, meaning we have to re-compute (i-1)/8 and (i-1)%8 in each loop? > > Maybe that's a bit slower? > Let's not worry about it. I think this is clearer. It's indeed cleaner. Can you check if it still looks good, then I will land. Thanks.
Patchset #4 (id:60001) has been deleted
On 2014/08/27 22:54:01, Bo Xu wrote: > Patchset #4 (id:60001) has been deleted Let's go with PS #2. I think that's clearest.
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as fdcc874 (presubmit successful).
Message was sent while issue was closed.
On 2014/08/27 22:56:01, Tom Sepez wrote: > On 2014/08/27 22:54:01, Bo Xu wrote: > > Patchset #4 (id:60001) has been deleted > Let's go with PS #2. I think that's clearest. OK. PS #2 landed, although #14 says PS #3 |
