You need to log in before you can comment on or make changes to this bug.
Created an attachment (id=366) [details] partial patch for issues described here Using quad-jpeg.tif from the standard test images, this dumps core: tiffcmp -l quad-jpeg.tif quad-jpeg.tif in either stock 3.9.2 or 3.9.2 plus the fixes discussed in bug #1936. I have been digging through this for a little bit, and what I find is that there are several issues around tiffcmp's attempt to read downsampled JPEG data via TIFFReadScanline without upsampling. (Since the problem examined in #1936 is mostly one of updating the desired buffer size after enabling upsampling, this seems like an independent bug, even though it comes from the same image type.) To wit: 1. The calculation of scan line size in TIFFScanlineSize() for the CONTIG YCBCR case is flat out wrong: it fails to account for the fact that if vertical subsampling > 1, a single "scanline" includes multiple rows of Y pixels. For example, quad-jpeg.tif has image width 512 and 2x2 sampling. The minimum readable unit is therefore two rows of 512 Y pixels plus one row of 256 Cb pixels and one row of 256 Cr pixels, or 1536 bytes altogether. But TIFFScanlineSize is reporting 1024. This causes tiffcmp.c to allocate too small a buffer. I am confused by the TIFFOldScanlineSize and TIFFNewScanlineSize variants and am not sure what the overall vision is here, but surely it can never be a good idea for TIFFScanlineSize to return less than the number of bytes TIFFReadScanline will emit. 2. JPEGDecodeRaw is written in such a way that it will read out an entire strip or tile per call, paying no attention whatever to the passed buffer size. This is surely bogus. 3. tiffcmp.c supposes that it should make "imagelength" calls to TIFFReadScanline, but this is not correct when the input data is vertically subsampled; there are only imagelength / ycbcrsampling[1] scanline clumps available. I have developed a partial patch that fixes #1 and #2 to the point that tiffcmp at least doesn't crash. The fix for #2 is incomplete though, because I get a lot of "Improper call to JPEG library in state 200" warnings, which I think is because JPEGDecodeRaw is calling TIFFjpeg_finish_decompress at the end of each strip. It probably shouldn't do that but I'm unsure what the design intent really is. I have not touched #3 because I wasn't sure how it would interact with the contig/separate logic in tiffcmp.
Tom, It is my opinion that the TIFFReadScanline() interface is not very close to operational for subsampled images returned raw, and I'm not too interested in fixing it. I would; however, like to see graceful error handling.
Well, you could certainly take the approach that this is all so broken it needs to be redone from the ground up :-(. My immediate interest is in having the available tools not crash, though. A larger point is that there are probably lots and lots of other programs out there that are using TIFFReadScanline in about as intelligent a way as tiffcmp is, and it would be a *real* good thing if they couldn't all be crashed so trivially. So I think it's important to fix #1 and #2 to the extent of making sure TIFFReadScanline does not overrun a buffer allocated according to what TIFFScanlineSize says. Figuring out what the image height should be taken to be is a lesser problem. You had muttered over in bug #1936 that we shouldn't be using JPEGDecodeRaw at all in this scenario, which I guess would mean forcing the results to be upsampled whether the caller asked for it or not. I guess that might be the safest solution but I don't understand the implications well enough to want to try it myself. In any case it seems like a rather large behavioral change for an allegedly stable branch.
If I'm not mistaken, Red Hat, SUSE, and debian (as of a few minute ago) have all accepted this patch as the fix to CVE-2010-3087. I'm not sure whether that influences anyone's decision as to whether to accept the patch as at least an interim solution. I also don't know what the status of this is in 4.0 (beta).
I've applied and committed the "partial patch" to the 3.9 branch. The patch does not apply at all to tif_strip.c in HEAD (4.0). However, the segfault does not occur there... [root@gollum libtiff]# tools/tiffcmp -l /tmp/libtiffpic/quad-jpeg.tif /tmp/libtiffpic/quad-jpeg.tif ; echo $? TIFFReadScanline: scanline oriented access is not supported for downsampled JPEG compressed images, consider enabling TIFF_JPEGCOLORMODE as JPEGCOLORMODE_RGB.. /tmp/libtiffpic/quad-jpeg.tif: EOF at scanline 0 0 [root@gollum libtiff]# I suspect that this result with HEAD is satisfactory since the aim of the 3.9 patching was to avoid the crash. Given that there are other issues in this bug report that are not limited to the crashing, I'll leave the bug report open.
I'm commenting here because I've encountered the error "TIFFReadScanline: scanline oriented access is not supported for downsampled JPEG compressed images, consider enabling TIFF_JPEGCOLORMODE as JPEGCOLORMODE_RGB" while using the Python library Pillow and I traced the error back to this line in libtiff: https://gitlab.com/libtiff/libtiff/blob/master/libtiff/tif_jpeg.c#L1491 For Pillow, the problem is being tracked at https://github.com/python-pillow/Pillow/issues/2926. I've spent most of the day trying to figure out how to enable TIFF_JPEGCOLORMODE as JPEGCOLORMODE_RGB, and I did achieve it, which prevented the error from occurring. However, when I try to convert the TIFF into other formats like JPEG and PNG, the colours are all wrong. I noticed though that the file format conversion works fine when I'm just using greyscale and not colour though. Looking at https://en.wikipedia.org/wiki/YCbCr, it looks like the grey Y image is working fine, but I'm getting the Cr (ie green/pink) image when I'm trying to get a RGB image. According to this line https://gitlab.com/libtiff/libtiff/blob/master/libtiff/tif_jpeg.c#L1276, the YCbCr should've been converted to RGB, so I don't know what's happening. I don't know if this is a case of a bug in libtiff or if it's a problem with how Pillow is using libtiff. I'm on Ubuntu 16.04.03 using the packaged version of libtiff5 which is 4.0.6-1. As for Pillow, I'm using the current development branch, and I've added my own block that looks like this: uint32 compression; TIFFGetField(tiff, TIFFTAG_COMPRESSION, &compression); if (compression && compression == COMPRESSION_JPEG){ TIFFSetField(tiff, TIFFTAG_JPEGCOLORMODE, JPEGCOLORMODE_RGB); }
Bugzilla is no longer used for tracking libtiff issues. Remaining open tickets, such as this one, have been migrated to the libtiff GitLab instance at https://gitlab.com/libtiff/libtiff/issues . The migrated tickets have their summary prefixed with [BZ#XXXX] where XXXX is the initial Bugzilla issue number.
