Bug 100144

Created attachment 170194 [details] Patch

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

Created attachment 170199 [details] Patch

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?

Created attachment 170219 [details] Patch

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.

(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.)

(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.

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?

(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". :)

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?

Created attachment 170222 [details] Patch for landing

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.

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

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.

Created attachment 170225 [details] Patch for landing

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)"""

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

Comment on attachment 170225 [details] Patch for landing Clearing flags on attachment: 170225 Committed r132260: <http://trac.webkit.org/changeset/132260>

All reviewed patches have been landed. Closing bug.

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

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. :(

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."

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).

(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.

(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.

(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.

(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?

(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)

(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.

(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.

(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)

(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.

(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.