AWARE [SYSTEMS] Imaging expertise for the Delphi developer
AWare Systems, Imaging expertise for the Delphi developer, Home TIFF and LibTiff Mailing List Archive

LibTiff Mailing List

TIFF and LibTiff Mailing List Archive
December 2016

Previous Thread
Next Thread

Previous by Thread
Next by Thread

Previous by Date
Next by Date

Contact

The TIFF Mailing List Homepage
This list is run by Frank Warmerdam
Archive maintained by AWare Systems



Valid HTML 4.01!



Thread

2016.12.17 15:15 "Thoughts about a proposed patch to fix a heap-based buffer overflow in putcontig8bitYCbCr44tile ?", by Even Rouault
2016.12.17 18:46 "Re: =?utf-8?q?Thoughts_about_a_proposed_patch_to_fix_a_heap-ba?= =?utf-8?q?sed_buffer=09overflow_in_putcontig8bitYCbCr44tile_=3F?=", by Mickey Rose
2016.12.17 20:35 "Re: Thoughts about a proposed patch to fix a heap-based buffer overflow in putcontig8bitYCbCr44tile ?", by Even Rouault

2016.12.17 18:46 "Re: =?utf-8?q?Thoughts_about_a_proposed_patch_to_fix_a_heap-ba?= =?utf-8?q?sed_buffer=09overflow_in_putcontig8bitYCbCr44tile_=3F?=", by Mickey Rose

---------- Původní zpráva ----------
Od: Even Rouault <even.rouault@spatialys.com>
Datum: 17. 12. 2016 16:21:33


"
I've spent a couple of hours analyzing the issue raised on 
http://bugzilla.maptools.org/show_bug.cgi?id=2637

 
 
This is a defect in tif_getimage.c when decoding a tiled old-JPEG

with YCbCr 4:4 resampling, and with a unusual tile width of 3.

"



I wonder how you managed to create tiled YCbCr tiff. Some time ago I wanted 
to create some

such images for testing, and couldn't figure out how, as the rgb2ycbcr tool 
only creates stripped,

and tiffcp and tiffcrop refuse to copy subsampled non-JPEG images.





But I digress. The function `putcontig8bitYCbCr44tile` is used for stripped 
images as well, so you

don't need to create images with weird tile sizes, you can just create 
stripped images with weird


widths and observe that 4x4 subsampling is a no-go with certain widths.



"

putcontig8bitYCbCr44tile has an optimized case when the useful width and 
height are

multiple of 4, and here we are in the general case when they are not. So 
apparently

this code is supposed to deal with useful width not multiple of 4. I cannot 
really

make sense of the (fromskew * 18) / 4 computation (18=4x4+2 and 4 must be 
the horizontal

or vertical resampling factor) and the pp += fromskew (note that

those are also used in the optimized case ). Interestingly in the 2x2 
resampling case,

the formula is (fromskew / 2) * 6, so the division there is made before the 
computation.

So perhaps a fix would be (fromskew / 4) * 18 ?, which would evaluate to 0 
here

and avoid the overflow.



"



`fromskew` that is passed to the function is the number of pixels to skip 
when advancing to the next row.

This calculation turns it into the number of bytes to skip when advancing to
the next 4-row pack.


In 4x4 subsampling case, there are 4x4 Y samples, 1 Cb and 1 Cr sample in a 
block, hence the block size is 18.


`(fromskew / 4) * 18` might fix that for the tile reading case, but not when
you ask TIFFRGBAImageGet
to give you w=19 pixels (with col_offset=0 to keep it simple) from a 
stripped image whose width=21;
because fromskew=2px, but it must skip the 1 right-most block when advancing
to the next row pack.

In other words, the number of skipped pixels alone isn't enough information 
to calculate how many bytes
need to be skipped.

I've recently re-written a considerable portion of `get_image.c` put* 
functions because I needed to extract
regions with arbitrary `col_offset` and `row_offset` from any kind of image.
Although partial support for
arbitrary `col_offset` was recently added to libtiff, it's still broken for 
paletted and subsampled images,
so I had to roll my own. I can turn my modifications into a proper patch, 
but that's not a priority for me and
I'd very much prefer to do that after libtiff will have migrated to git or 
mercurial.

After having implemented and tested arbitrary region extraction from 4x4 
subsampled images, I came to
the conclusion that it'd be better to remove 4x4 subsampling support from 
libtiff and the TIFF specification
as well. I was planning to propose that in a message of its own, as it sure 
will be contentious, but since you
mentioned one of the reasons, I think I might just add to that...
 "

But I cannot verify if that doesnt break valid images. Has 

someone legit OJPEG tiled 4:4 images... ? I tried to create a regular JPEG 
tiled 4:4, with

tile of dimension 32x32 (otherwise this is rejected), and of image dimension
63x63, but this

is kind of fun since by default libjpeg has a C_MAX_BLOCKS_IN_MCU/D_MAX_
BLOCKS_IN_MCU limit

to 10 that prevents that (you need to increase it to 20 for example)

"



Yes, and there's also a snarly comment in libjpeg source, discouraging from 
increasing the

compression limit (for decompression you might be *forced* to increase it to
decode an image

you get somewhere).




Therefore it is impossible to create or decode a JPEG-compressed TIFF with 4
x4 subsampling

(which would require 18 blocks in MCU), unless you use a custom-built 
libjpeg with higher limits.

IMO not worth it, totally defeats portability.





So you have to use a different compression if you want 4x4 subsampling. Now,
I'm no expert

on image compression, but isn't it generally better to subsample less and 
lossy-compress more?

I mean, if I had a source (non-subsampled) image and a desired subsampled 
file size, it'd expect

4x2-subsampled JPEG to look better than 4x4-subsampled ANOTHER-compression, 
simply

because the 4x2 JPEG might preserve some detail (if the size budget allows 
sufficient quality)

that the 4x4 has lost even before compression.





The second reason I think 4x4 subsampling is not worth supporting is that it
complicates

implementation. Compare `putcontig8bitYCbCr44tile` with any of the other `
put*` routines.

And it grows larger with correct handling of arbitrary col_offset. Of 
course, that's just a few

more lines of code, so not a big deal.





But there's a bigger issue. Here's a part of `TIFFScanlineSize64` comment 
from `tif_strip.c`


 * The ScanlineSize in case of YCbCrSubsampling is defined as the
 * strip size divided by the strip height, i.e. the size of a pack of 
vertical
 * subsampling lines divided by vertical subsampling. It should thus make
 * sense when multiplied by a multiple of vertical subsampling.

In the case of 4x4 subsampling (18-byte blocks), this definition only makes 
sense
when the number of blocks per line is even. When there's an odd number of 
them,
the size of the pack isn't divisible by 4.

If (1 <= width % 8  <= 4) then ScanlineSize is not a whole number.

Hence 4x4 subsampled images would need special treatment wherever 
ScanlineSize
is used to calculate some offset into the image.