Hi Brian, > > Under certain conditions, the td->td_transferfunction table might not > > have the excepted size, that is it may not have the excepted number of > > samples per pixel (td->td_samplesperpixel). In this case for example, > > the table is only 3 rows large while td->td_samplesperpixel is 4. Then, > > the program segfaults when it comes to td->td_transferfunction[3][0]. > > The faulty test case is where the table is suppose to have three > entries, but only two entries are provided.
Sure ? To me it looked like three entries are provided, but the fourth
(td->td_transferfunction[3]) isn't.
$ ASAN_OPTIONS=abort_on_error=1 gdb tiffinfo
[snip]
(gdb) r -c ../1-tiffinfo-c-null
Starting program: /usr/local/bin/tiffinfo -c ../1-tiffinfo-c-null
[Thread debugging using libthread_db enabled]
Using host libthread_db library
"/lib/x86_64-linux-gnu/libthread_db.so.1".
TIFFReadDirectoryCheckOrder: Warning, Invalid TIFF directory; tags are
not sorted in ascending order.
TIFFReadDirectory: Warning, Unknown field with tag 314 (0x13a)
encountered.
TIFFReadDirectory: Warning, Unknown field with tag 54034 (0xd312)
encountered.
TIFFFetchNormalTag: Warning, Incorrect count for "YResolution"; tag
ignored.
TIFF Directory at offset 0x767fe (485374)
Image Width: 1024 Image Length: 768
Resolution: 2, 0 (unitless)
Bits/Sample: 8
Compression Scheme: LZW
Photometric Interpretation: RGB color
Samples/Pixel: 4
Planar Configuration: single image plane
Transfer Function:
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff6bb1da1 in TIFFPrintDirectory (tif=0x619000000080,
fd=0x7ffff5ed8600 <_IO_2_1_stdout_>, flags=6) at tif_print.c:551
551 td->td_transferfunction[i][l]);
(gdb) l
546 uint16 i;
547 fprintf(fd, " %2ld: %5u",
548 l, td->td_transferfunction[0][l]);
549 for (i = 1; i < td->td_samplesperpixel; i++)
550 fprintf(fd, " %5u",
551 td->td_transferfunction[i][l]);
552 fputc('\n', fd);
553 }
554 } else
555 fprintf(fd, "(present)\n");
(gdb) p td->td_transferfunction
$1 = {0x615000000580, 0x615000000800, 0x615000000a80}
> The defintion of td_transferfunction is:
>
> typedef struct {
> ...
> uint16* td_transferfunction[3];
> ...
> } TIFFDirectory;
>
> My assumption was that the list would be NULL terminated. In practise it
> is NULL terminated (might be accidental due to newly allocated memory
> being initialized to 0), but I should double check this.
I think we should really avoid relying on NULL termination for this
patch, because nothing guarantees us that this fourth element has been
initialized to 0 at any moment, right ?
> However, as this table is only 3 entries long (huh? Is that hardcoded
> value really safe here?), so it cannot be null terminated for the case
> where there are three tables.
Hum well, actually the specification states:
The TransferFunction can be applied to images with a
PhotometricInterpretation value of RGB, Palette, YCbCr, WhiteIsZero,
and BlackIsZero. The TransferFunction is not used with other
PhotometricInterpretation types.
TIFF 6.0 Specification, p84[0]
Palette => td->td_samplesperpixel = 1
YCbCr => td->td_samplesperpixel = 3
WhiteIsZero => td->td_samplesperpixel = 1
RGB => td->td_samplesperpixel = 3 (unless ExtraSamples specified, we
will take a look at it later)
So, in fact it may very well be that the size of the TransferFunction table
is always at most 3 rows and this definition is right.
However, there is still the case of RGB where we may still have
td->td_samplesperpixel > 3 (this is our case, here td->td_samplesperpixel = 4
and td->td_photometric = 2 which is RGB).
There are two possibilities
(1) ExtraSamples = 1 => Associated alpha data (with pre-multiplied color)
(2) ExtraSamples = 2 => Unassociated alpha data
I didn't have time to read all information about these two cases, but at
a first glance I noticed something interesting.
The specification states:
Since the ExtraSamples field is independent of other fields, this
scheme permits alpha information to be stored in whatever organization
is appropriate. In particular, components can be stored packed
(PlanarConfiguration=1); [... snip] However, if this scheme is used,
TIFF readers must not derive the SamplesPerPixel from the value of the
PhotometricInterpretation field (e.g., if RGB, then SamplesPerPixel is
3).
TIFF 6.0 Specification, p78[0]
In our case, while components are stored packed (because td->td_planarconfig =
1),
the td->td_samplesperpixel is still 4 and not 3, so the specification isn't
really respected, right ?
> However, it still could be null terminated for less then three entries.
Maybe, but if I don't misunderstand the problem, the bug is not affecting
us if td->td_samplesperpixel < 3 because there is no inconsistency anymore,
that is td->td_samplesperpixel is not bigger than the td_transferfunction
array.
Cheers,
Hugo
[0] https://www.itu.int/itudoc/itu-t/com16/tiff-fx/docs/tiff6.pdf
--
Hugo Lefeuvre (hle) | www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
signature.asc
Description: PGP signature
