libexif: Fix out of bound reads in exif_mnote_data_olympus_load().

libexif: Fix out of bound reads in exif_mnote_data_olympus_load().

BUG=576984
R=rickyz@chromium.org

Committed: https://chromium.googlesource.com/chromium/deps/libexif/sources/+/045b7fb9aa6d9b7f1954db248caf5eefe917476d

[Lei Zhang, 2016-01-13 06:36:35]

thestig@chromium.org changed reviewers:
+ rickyz@chromium.org

[Lei Zhang, 2016-01-13 06:36:36]

Assigner gets the code review.

BTW, we are trying to stop using libexif for Chromium, but it would be nice to
fix bugs in it. I have it installed locally and have a surprising number of apps
depend on it.

[rickyz (no longer on Chrome), 2016-01-13 21:33:23]

Thanks for the super quick fix! lgtm with some small comments

https://codereview.chromium.org/1585593002/diff/1/libexif/olympus/exif-mnote-...
File libexif/olympus/exif-mnote-data-olympus.c (right):

https://codereview.chromium.org/1585593002/diff/1/libexif/olympus/exif-mnote-...
libexif/olympus/exif-mnote-data-olympus.c:42: return num1 > (size_t)-1 - num2 ?
1 : 0;
Can this use SIZE_MAX from stdint.h instead, and is the ternary operator needed?

https://codereview.chromium.org/1585593002/diff/1/libexif/olympus/exif-mnote-...
libexif/olympus/exif-mnote-data-olympus.c:45: static int exceeds_buffer(size_t
buf_size, size_t index, size_t offset)
nit: Maybe the names (size_t buf_size, size_t offset, size_t len) make more
sense?

https://codereview.chromium.org/1585593002/diff/1/libexif/olympus/exif-mnote-...
libexif/olympus/exif-mnote-data-olympus.c:47: return buf_size < offset || index
> buf_size - offset ? 1 : 0;
Should the two comparisons be inclusive here?

[Lei Zhang, 2016-01-13 22:02:20]

https://codereview.chromium.org/1585593002/diff/1/libexif/olympus/exif-mnote-...
File libexif/olympus/exif-mnote-data-olympus.c (right):

https://codereview.chromium.org/1585593002/diff/1/libexif/olympus/exif-mnote-...
libexif/olympus/exif-mnote-data-olympus.c:42: return num1 > (size_t)-1 - num2 ?
1 : 0;
On 2016/01/13 21:33:23, rickyz wrote:
> Can this use SIZE_MAX from stdint.h instead, and is the ternary operator
needed?

I'm hoping this is something that upstream will take. I'm not sure if SIZE_MAX
is always available.

The ternary operator is probably not needed.

https://codereview.chromium.org/1585593002/diff/1/libexif/olympus/exif-mnote-...
libexif/olympus/exif-mnote-data-olympus.c:45: static int exceeds_buffer(size_t
buf_size, size_t index, size_t offset)
On 2016/01/13 21:33:23, rickyz wrote:
> nit: Maybe the names (size_t buf_size, size_t offset, size_t len) make more
> sense?

Done.

https://codereview.chromium.org/1585593002/diff/1/libexif/olympus/exif-mnote-...
libexif/olympus/exif-mnote-data-olympus.c:47: return buf_size < offset || index
> buf_size - offset ? 1 : 0;
On 2016/01/13 21:33:22, rickyz wrote:
> Should the two comparisons be inclusive here?

I don't think so, exceeds_buffer(10, 0, 10) should return false, right?

[rickyz (no longer on Chrome), 2016-01-13 22:08:47]

lgtm

https://codereview.chromium.org/1585593002/diff/1/libexif/olympus/exif-mnote-...
File libexif/olympus/exif-mnote-data-olympus.c (right):

https://codereview.chromium.org/1585593002/diff/1/libexif/olympus/exif-mnote-...
libexif/olympus/exif-mnote-data-olympus.c:42: return num1 > (size_t)-1 - num2 ?
1 : 0;
On 2016/01/13 at 22:02:19, Lei Zhang wrote:
> On 2016/01/13 21:33:23, rickyz wrote:
> > Can this use SIZE_MAX from stdint.h instead, and is the ternary operator
needed?
> 
> I'm hoping this is something that upstream will take. I'm not sure if SIZE_MAX
is always available.
> 
> The ternary operator is probably not needed.

Ah, didn't realize SIZE_MAX was only introduced in C99, makes sense.

https://codereview.chromium.org/1585593002/diff/1/libexif/olympus/exif-mnote-...
libexif/olympus/exif-mnote-data-olympus.c:47: return buf_size < offset || index
> buf_size - offset ? 1 : 0;
On 2016/01/13 at 22:02:20, Lei Zhang wrote:
> On 2016/01/13 21:33:22, rickyz wrote:
> > Should the two comparisons be inclusive here?
> 
> I don't think so, exceeds_buffer(10, 0, 10) should return false, right?

Oops, never mind, I was confused by the argument names and thought the last two
arguments were indexes - this looks right.

[Lei Zhang, 2016-01-13 22:30:02]

Description was changed from

==========
libexif: Fix out of bound reads in exif_mnote_data_olympus_load().

BUG=576984
==========

to

==========
libexif: Fix out of bound reads in exif_mnote_data_olympus_load().

BUG=576984
R=rickyz@chromium.org

Committed:
https://chromium.googlesource.com/chromium/deps/libexif/sources/+/045b7fb9aa6...
==========

[Lei Zhang, 2016-01-13 22:30:03]

Committed patchset #2 (id:20001) manually as
045b7fb9aa6d9b7f1954db248caf5eefe917476d (presubmit successful).