Bug 100144 – In the open-source jpeg decoder, read image orientation from the exif data

• Component: New Bugs
• Status: RESOLVED FIXED

[Nico Weber]

In the open-source jpeg decoder, read image orientation from the exif data

[Nico Weber]

Created attachment 170194 [details]
Patch

[WebKit Review Bot]

Comment on attachment 170194 [details]
Patch

Attachment 170194 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14529039

[Nico Weber]

Created attachment 170199 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 170199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170199&action=review

I think the primary concern with patches like this is readability.  I think you've done a fine job, but I listed a few notes below which might help make it even easier to understand what's going on here.  (Which helps us make sure there are no security holes or errors in the future.)

One more round please. :)

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:151
> +static bool
> +checkExifHeader(jpeg_saved_marker_ptr marker, bool* isBigEndian, unsigned* idfOffset)

I believe these normally go on the same line in webKit.

If isBigEndian/idfOffset aren't optional, seems they should be & instead of *.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:156
> +    // 'M', 'M' (motorola / big endian byte order), folled by (uint16_t)42,

folled

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:166
> +        && marker->data[0] == 'E'
> +        && marker->data[1] == 'x'
> +        && marker->data[2] == 'i'
> +        && marker->data[3] == 'f'
> +        && marker->data[4] == '\0'

We have some (primative) utilities for string-based parsing (as opposed to character-based, as I might describe this approach) in ParserUtilities.h.  But I suspect the type of marker->data is not UChar*, so even those primitives won't work here.  The CSSParser is another example of a UChar* parser in WebKit.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:189
> +        idfOffset += 6; // Account for 'Exif\0\0' header.

You could also use sizeof("Exif\0") if you wanted.  I'm not sure it's more clear in this instance until you're using string-based parsing throught the rest of the code.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:190
> +        if (idfOffset >= marker->data_length - 2)

No risk of underflow of an unsigned, right?

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:192
> +        JOCTET* idf = marker->data + idfOffset;

idf?

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:195
> +        idf += 2;

What are we skipping here?

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:197
> +        for (unsigned i = 0; i < tagCount && end - idf >= 12; ++i, idf += 12) {

12 should probably have a name.  Do you want to give a comment description of the layout of what you're reading here?

[Nico Weber]

Created attachment 170219 [details]
Patch

[Nico Weber]

Thanks for the quick review!

(In reply to comment #4)
> > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:151
> > +static bool
> > +checkExifHeader(jpeg_saved_marker_ptr marker, bool* isBigEndian, unsigned* idfOffset)
> 
> I believe these normally go on the same line in webKit.

Done.

> If isBigEndian/idfOffset aren't optional, seems they should be & instead of *.
> 
> > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:156
> > +    // 'M', 'M' (motorola / big endian byte order), folled by (uint16_t)42,
> 
> folled

Done.

> > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:166
> > +        && marker->data[0] == 'E'
> > +        && marker->data[1] == 'x'
> > +        && marker->data[2] == 'i'
> > +        && marker->data[3] == 'f'
> > +        && marker->data[4] == '\0'
> 
> We have some (primative) utilities for string-based parsing (as opposed to character-based, as I might describe this approach) in ParserUtilities.h.  But I suspect the type of marker->data is not UChar*, so even those primitives won't work here.  The CSSParser is another example of a UChar* parser in WebKit.

Right, JOCTET is defined to either `unsigned char` or `char`, depending on a jpeglib configuration option (else I'd just used strncmp()).

> 
> > +        idfOffset += 6; // Account for 'Exif\0\0' header.
> 
> You could also use sizeof("Exif\0") if you wanted.  I'm not sure it's more clear in this instance until you're using string-based parsing throught the rest of the code.

Left as is.

> > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:190
> > +        if (idfOffset >= marker->data_length - 2)
> 
> No risk of underflow of an unsigned, right?

I tweaked this to be more like the change in the loop.

> > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:192
> > +        JOCTET* idf = marker->data + idfOffset;
> 
> idf?

Err, I meant "ifd". That's from the tiff spec and is short for "image file directory". I can rename it to something else ("directory") if you like, but without the tiff spec at hand this code probably shouldn't be changed anyway.

> > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:195
> > +        idf += 2;
> 
> What are we skipping here?

Added a comment.

> > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:197
> > +        for (unsigned i = 0; i < tagCount && end - idf >= 12; ++i, idf += 12) {
> 
> 12 should probably have a name.  Do you want to give a comment description of the layout of what you're reading here?

Done.

[Eric Seidel (no email)]

(In reply to comment #6)
> Err, I meant "ifd". That's from the tiff spec and is short for "image file directory". I can rename it to something else ("directory") if you like, but without the tiff spec at hand this code probably shouldn't be changed anyway.

Seems like we should be sure to note that somewhere.  Ideally with a link to the spec.

(It's possible you've already done that, I haven't pulled up your patch yet.  I'll do that in a few.)

[Nico Weber]

(In reply to comment #7)
> (In reply to comment #6)
> > Err, I meant "ifd". That's from the tiff spec and is short for "image file directory". I can rename it to something else ("directory") if you like, but without the tiff spec at hand this code probably shouldn't be changed anyway.
> 
> Seems like we should be sure to note that somewhere.  Ideally with a link to the spec.
> 
> (It's possible you've already done that, I haven't pulled up your patch yet.  I'll do that in a few.)

Yes, I mention this in a comment now.

[Eric Seidel (no email)]

Comment on attachment 170219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170219&action=review

This is definitely more readable to my eyes.  Thank you. Helps that I read it twice, I'm sure. :)

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:166
> +        // data[5] is a fill byte

What's a fill byte?

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:188
> +        ifdOffset += 6; // Account for 'Exif\0\0' header.

Why Exif\0\0 here and only Exif\0 above?

[Eric Seidel (no email)]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Err, I meant "ifd". That's from the tiff spec and is short for "image file directory". I can rename it to something else ("directory") if you like, but without the tiff spec at hand this code probably shouldn't be changed anyway.
> > 
> > Seems like we should be sure to note that somewhere.  Ideally with a link to the spec.
> > 
> > (It's possible you've already done that, I haven't pulled up your patch yet.  I'll do that in a few.)
> 
> Yes, I mention this in a comment now.

The part of that statement I was actually interested in was "without the tiff spec at hand this code probably shouldn't be changed anyway". :)

[Alexey Proskuryakov]

Comment on attachment 170219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170219&action=review

> Source/WebCore/ChangeLog:10
> +        setting (see bug 19688). Currently this isn't hooked up anywhere, so
> +        it has no observable effect for now.

Could you please clarify? Are you saying that the setting can not be enabled in any port that that uses open source decoders?

[Nico Weber]

Created attachment 170222 [details]
Patch for landing

[Nico Weber]

Thanks!


Added a link to the spec too.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:166
>> +        // data[5] is a fill byte
> 
> What's a fill byte?

It's a byte whose value doesn't matter. In practice, it seems to be 0 or 0xff, so I suppose encoders can put whatever they want there.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:188
>> +        ifdOffset += 6; // Account for 'Exif\0\0' header.
> 
> Why Exif\0\0 here and only Exif\0 above?

It's "Exif\0<fill byte>". Changed the comment to say that.

[WebKit Review Bot]

Comment on attachment 170222 [details]
Patch for landing

Rejecting attachment 170222 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14526087

[Nico Weber]

Comment on attachment 170219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170219&action=review

Added a link to the spec too.

>> Source/WebCore/ChangeLog:10
>> +        it has no observable effect for now.
> 
> Could you please clarify? Are you saying that the setting can not be enabled in any port that that uses open source decoders?

It can be enabled, but it currently has no effect. I'm trying to change that. To change that, I need to

1) Let the jpeg decoder read orientation metadata from the file (done for exif in this patch)
2) Change the image source code to grab orientation data from the decoder and pass it on the BitmapImage (like ImageSourceCG does) (next patch)
3) Update the drawing code for the chromium port to actually honor the orientation on BitmapImage (patch after next patch)
4) Turn on shouldRespectImageOrientation for the chromium port
( 5. Update the clipboard writing code to honor the orientation bit)

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:166
>> +        // data[5] is a fill byte
> 
> What's a fill byte?

It's a byte whose value doesn't matter. In practice, it seems to be 0 or 0xff, so I suppose encoders can put whatever they want there.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:188
>> +        ifdOffset += 6; // Account for 'Exif\0\0' header.
> 
> Why Exif\0\0 here and only Exif\0 above?

It's "Exif\0<fill byte>". Changed the comment to say that.

[Nico Weber]

Created attachment 170225 [details]
Patch for landing

[Nico Weber]

ap: Sorry, the in-patch publish button doesn't work for me for some reason. Instead replying on a comment:

Question: """
> Source/WebCore/ChangeLog:10
> +        setting (see bug 19688). Currently this isn't hooked up anywhere, so
> +        it has no observable effect for now.

Could you please clarify? Are you saying that the setting can not be enabled in any port that that uses open source decoders?
"""

Answer:
"""It can be enabled, but it currently has no effect. I'm trying to change that. To change that, I need to

1) Let the jpeg decoder read orientation metadata from the file (done for exif in this patch)
2) Change the image source code to grab orientation data from the decoder and pass it on the BitmapImage (like ImageSourceCG does) (next patch)
3) Update the drawing code for the chromium port to actually honor the orientation on BitmapImage (patch after next patch)
4) Turn on shouldRespectImageOrientation for the chromium port
( 5. Update the clipboard writing code to honor the orientation bit)"""

[Nico Weber]

Bleh, now it made it through. Sorry for the redundant comment.

[WebKit Review Bot]

Comment on attachment 170225 [details]
Patch for landing

Clearing flags on attachment: 170225

Committed r132260: <http://trac.webkit.org/changeset/132260>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

Was there such a hurry?  Patch landed before I got a chance to review :/ my TZ is not the same as yours.

[Eric Seidel (no email)]

I don't think there is any hurry.  Feel free to roll-out, or we can discuss now post-landing and land amendments.  I'm sure Nico would have no such objections.  I forgot you were working on EXIF specifically, this was one of many patches I reviewed today. :(

[noel gordon]

Comment on attachment 170225 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=170225&action=review

Thanks for working on this.  Some initial comments.


> Source/WebCore/platform/graphics/BitmapImage.h:208
> +    virtual void draw(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect, ColorSpace styleColorSpace, CompositeOperator, RespectImageOrientationEnum) OVERRIDE;

OVERRIDE?  Not following, did you change BitmapImage.cpp somehow in this patch?

> Source/WebCore/platform/image-decoders/ImageDecoder.h:303
> +        ImageOrientation orientation() const { return m_orientation; }

Do GIF or PNG decoders support orientation?  I think not, so why add m_orientation to all decoders?

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:101
> +

Just use JPEG_APP0 + 1 at the call site and remove these lines.  There's even a comment at the call site.  Make that comment exact: "Retain EXIF metadata marker APP1 for orientation detection."

[Nico Weber]

Thanks for taking a look!

(In reply to comment #23)
> (From update of attachment 170225 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170225&action=review
> > Source/WebCore/platform/graphics/BitmapImage.h:208
> > +    virtual void draw(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect, ColorSpace styleColorSpace, CompositeOperator, RespectImageOrientationEnum) OVERRIDE;
> 
> OVERRIDE?  Not following, did you change BitmapImage.cpp somehow in this patch?

This is mostly unrelated to the rest of the patch, it's just documentation. I didn't understand how this worked at first (see https://bugs.webkit.org/show_bug.cgi?id=90755#c6), so I added this as a comment for myself. It has no effect.

> > Source/WebCore/platform/image-decoders/ImageDecoder.h:303
> > +        ImageOrientation orientation() const { return m_orientation; }
> 
> Do GIF or PNG decoders support orientation?  I think not, so why add m_orientation to all decoders?

Technically, gif and png files can contain XMP metadata (http://partners.adobe.com/public/developer/en/xmp/sdk/XMPspecification.pdf), but we probably won't want to support this.

Alternatively, I could make orientation() virtual and override it in the jpeg decoder. That costs 4 byte for a vtable field too though, so it didn't seem simpler or more efficient to me. If you strongly prefer a virtual method and having this field just in the jpeg decoder, I can move it around. I don't care too much either way.

> > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:101
> > +
> 
> Just use JPEG_APP0 + 1 at the call site and remove these lines.  There's even a comment at the call site.  Make that comment exact: "Retain EXIF metadata marker APP1 for orientation detection."

The marker is used in two places, do you want me to put the same expression there?

I did the comment change and uploaded the diff to https://bugs.webkit.org/show_bug.cgi?id=100181 (that's where I'll add the other changes I'll do in response to your comments on this review too).

[noel gordon]

(In reply to comment #24)
> > OVERRIDE?  Not following, did you change BitmapImage.cpp somehow in this patch?
> 
> This is mostly unrelated to the rest of the patch, it's just documentation. I didn't understand how this worked at first (see https://bugs.webkit.org/show_bug.cgi?id=90755#c6), so I added this as a comment for myself. It has no effect.

Right, understood.

[noel gordon]

(In reply to comment #24)
> > Do GIF or PNG decoders support orientation?  I think not, so why add m_orientation to all decoders?
> 
> Technically, gif and png files can contain XMP metadata (http://partners.adobe.com/public/developer/en/xmp/sdk/XMPspecification.pdf), but we probably won't want to support this.

I've never seen a GIF or PNG with XMP metadata in the field, so let's not worry about it for now.

[noel gordon]

(In reply to comment #24)
> Alternatively, I could make orientation() virtual and override it in the jpeg decoder. That costs 4 byte for a vtable field too though, so it didn't seem simpler or more efficient to me. If you strongly prefer a virtual method and having this field just in the jpeg decoder, I can move it around. I don't care too much either way.

The image decoders are virtual and override all over :)  Thinking more about David's work mentioned in bug 100191, let's leave it as is.

[noel gordon]

(In reply to comment #24)
> The marker is used in two places, do you want me to put the same expression there?

Yes please.
 
> I did the comment change and uploaded the diff to https://bugs.webkit.org/show_bug.cgi?id=100181 (that's where I'll add the other changes I'll do in response to your comments on this review too).

Thank you.  I need to look closer at the exif reader.  The image decoders have been a source of security flaws in the past so we really need the code to be dumb-simple. 

A general question is what to do when there are any invalid EXIF tags, or two or more valid EXIF orientation tags (as eric and roc@mozilla discussed in bug 19688).  Should we just ignore orientation in these cases?  How does your EXIF reader implementation handle these cases?

[Nico Weber]

(In reply to comment #28)
> (In reply to comment #24)
> > The marker is used in two places, do you want me to put the same expression there?
> 
> Yes please.

Added to the patch in bug 100181.

> Thank you.  I need to look closer at the exif reader.  The image decoders have been a source of security flaws in the past so we really need the code to be dumb-simple. 

Thanks for taking a second look at this.

> A general question is what to do when there are any invalid EXIF tags, or two or more valid EXIF orientation tags (as eric and roc@mozilla discussed in bug 19688).  Should we just ignore orientation in these cases?  How does your EXIF reader implementation handle these cases?

The code is right here, in this patch :-) It keeps the last orientation tag from the last APP1 section. http://people.mozilla.org/~roc/IMG_20110526_110158.jpg (from the other bug) shows up as upright (roc said that this means we're using the last tag too). Safari shows that image upright too (however, ignoring the rotation tag for that image would show it upright as well, so I'm not certain if Safari looks at the last tag or ignores the tag when there are multiple)

[noel gordon]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #24)
> > > The marker is used in two places, do you want me to put the same expression there?
> > 
> > Yes please.
> 
> Added to the patch in bug 100181.

OK.

> > Thank you.  I need to look closer at the exif reader.  The image decoders have been a source of security flaws in the past so we really need the code to be dumb-simple. 
> 
> Thanks for taking a second look at this.

Asked chrome-security to look while I was absent, bug 100320.

[noel gordon]

(In reply to comment #29)
> > A general question is what to do when there are any invalid EXIF tags, or two or more valid EXIF orientation tags (as eric and roc@mozilla discussed in bug 19688).  Should we just ignore orientation in these cases?  How does your EXIF reader implementation handle these cases?
> 
> The code is right here, in this patch :-) 

The code is right there indeed:

   if (tag == orientationTag && type == shortType && count == 1)
      return ImageOrientation::fromEXIFValue(readUint16(ifd + 8, isBigEndian));


The "return" suggests the code uses the first orientation tag within an APP1, right?  Doesn't match the description:

> It keeps the last orientation tag from the last APP1 section.

[Nico Weber]

(In reply to comment #31)
> (In reply to comment #29)
> > > A general question is what to do when there are any invalid EXIF tags, or two or more valid EXIF orientation tags (as eric and roc@mozilla discussed in bug 19688).  Should we just ignore orientation in these cases?  How does your EXIF reader implementation handle these cases?
> > 
> > The code is right here, in this patch :-) 
> 
> The code is right there indeed:
> 
>    if (tag == orientationTag && type == shortType && count == 1)
>       return ImageOrientation::fromEXIFValue(readUint16(ifd + 8, isBigEndian));
> 
> 
> The "return" suggests the code uses the first orientation tag within an APP1, right?  Doesn't match the description:
> 
> > It keeps the last orientation tag from the last APP1 section.

It keeps the first tag from the first APP1 section then. Since http://people.mozilla.org/~roc/IMG_20110526_110158.jpg shows up fine, I suppose roc's description of that image was off too. (I tried changing the code to look at the last tag instead -- only one marker makes it through in that case too. Either libjpeg strips duplicate app1 markers -- jdmarker.c looks like it doesn't -- or the second orientation tag is in a later ifd)

[Nico Weber]

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #29)
> > > > A general question is what to do when there are any invalid EXIF tags, or two or more valid EXIF orientation tags (as eric and roc@mozilla discussed in bug 19688).  Should we just ignore orientation in these cases?  How does your EXIF reader implementation handle these cases?
> > > 
> > > The code is right here, in this patch :-) 
> > 
> > The code is right there indeed:
> > 
> >    if (tag == orientationTag && type == shortType && count == 1)
> >       return ImageOrientation::fromEXIFValue(readUint16(ifd + 8, isBigEndian));
> > 
> > 
> > The "return" suggests the code uses the first orientation tag within an APP1, right?  Doesn't match the description:
> > 
> > > It keeps the last orientation tag from the last APP1 section.
> 
> It keeps the first tag from the first APP1 section then. Since http://people.mozilla.org/~roc/IMG_20110526_110158.jpg shows up fine, I suppose roc's description of that image was off too. (I tried changing the code to look at the last tag instead -- only one marker makes it through in that case too. Either libjpeg strips duplicate app1 markers -- jdmarker.c looks like it doesn't -- or the second orientation tag is in a later ifd)

I changed the code locally to look at more than the first ifd, and roc's image does have a 2nd orientation marker in a 2nd ifd. From my reading of the spec, that 2nd ifd describes an optional embedded thumbnail though, and that 2nd ifd does contain "width = 320, height = 240, has embedded jpeg data = 1". So it looks like roc tried to apply the orientation flag from the embedded thumbnail to the main image.

[noel gordon]

(In reply to comment #33)
> > It keeps the first tag from the first APP1 section then. 

It reads the orientation tag from IFD0.  That looks correct to me.

> > Since http://people.mozilla.org/~roc/IMG_20110526_110158.jpg shows up fine, I suppose roc's description of that image was off too. (I tried changing the code to look at the last tag instead -- only one marker makes it through in that case too. Either libjpeg strips duplicate app1 markers -- jdmarker.c looks like it doesn't -- or the second orientation tag is in a later ifd)

The second orientation tag is in IFD1, refer to this picture at

http://www.media.mit.edu/pia/Research/deepview/exif.html#ExifData

> I changed the code locally to look at more than the first ifd, and roc's image does have a 2nd orientation marker in a 2nd ifd. From my reading of the spec, that 2nd ifd describes an optional embedded thumbnail though, and that 2nd ifd does contain "width = 320, height = 240, has embedded jpeg data = 1". So it looks like roc tried to apply the orientation flag from the embedded thumbnail to the main image.

and I agree, IFD1 is used to describe the EXIF of the thumbnail image.

With bug 100320 done, the reader seems secure and sensible.