code review 4815061: image/tiff: Do not panic when RowsPerStrip is missing.

image/tiff: Do not panic when RowsPerStrip is missing.

The RowsPerStrip tag is mandatory according to the spec. However,
Mac OS sometimes (?) omits it. I managed to create such an image
by applying "tiffutil -none" on a compressed image.

If RowsPerStrip is 0, there was a division by zero in the decoder.
Assume that the image is a single strip in this case.

[bsiegert, 2011-07-26 17:50:43]

Hello nigeltao@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-07-26 17:54:30]

Add the image you created (I assume it's small?) to the test data?


On Tue, Jul 26, 2011 at 10:50 AM, <bsiegert@gmail.com> wrote:

> Reviewers: nigeltao,
>
> Message:
> Hello nigeltao@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> image/tiff: Do not panic when RowsPerStrip is missing.
>
> The RowsPerStrip tag is mandatory according to the spec. However,
> Mac OS sometimes (?) omits it. I managed to create such an image
> by applying "tiffutil -none" on a compressed image.
>
> If RowsPerStrip is 0, there was a division by zero in the decoder.
> Assume that the image is a single strip in this case.
>
> Please review this at
http://codereview.appspot.com/**4815061/<http://codereview.appspot.com/4815061/>
>
> Affected files:
>  M src/pkg/image/tiff/reader.go
>
>
> Index: src/pkg/image/tiff/reader.go
> ==============================**==============================**=======
> --- a/src/pkg/image/tiff/reader.go
> +++ b/src/pkg/image/tiff/reader.go
> @@ -362,6 +362,10 @@
>
>        // Check if we have the right number of strips, offsets and counts.
>        rps := int(d.firstVal(tRowsPerStrip))
> +       if rps == 0 {
> +               // Assume only one strip.
> +               rps = d.config.Height
> +       }
>        numStrips := (d.config.Height + rps - 1) / rps
>        if rps == 0 || len(d.features[tStripOffsets]) < numStrips ||
> len(d.features[**tStripByteCounts]) < numStrips {
>                return nil, FormatError("inconsistent header")
>
>
>

[bsiegert, 2011-07-26 18:14:13]

On Tue, Jul 26, 2011 at 19:54, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Add the image you created (I assume it's small?) to the test data?

It is quite big actually. I will create a smaller one and add it.

--Benny.


-- 
The first essential in chemistry is that you should perform practical
work and conduct experiments, for he who performs not practical work
nor makes experiments will never attain the least degree of mastery.
        -- Abu Musa Jabir ibn Hayyan (721-815)

[bsiegert, 2011-07-26 19:40:27]

Hello nigeltao@golang.org, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[nigeltao, 2011-07-27 02:22:07]

http://codereview.appspot.com/4815061/diff/3006/src/pkg/image/tiff/reader_tes...
File src/pkg/image/tiff/reader_test.go (right):

http://codereview.appspot.com/4815061/diff/3006/src/pkg/image/tiff/reader_tes...
src/pkg/image/tiff/reader_test.go:12: // This test tries to decode an image that
has no RowsPerStrip tag.
s/This test/TestNoRPS/.

http://codereview.appspot.com/4815061/diff/3006/src/pkg/image/tiff/reader_tes...
src/pkg/image/tiff/reader_test.go:16: f, err := os.Open("testdata/rps0.tiff")
If the func is called TestNoRPS, then call the test image
"testdata/no_rps.tiff".

http://codereview.appspot.com/4815061/diff/3006/src/pkg/image/tiff/reader_tes...
src/pkg/image/tiff/reader_test.go:18: t.Errorf(err.String())
t.Error(err)
return

Same below.

[bsiegert, 2011-07-27 18:55:12]

Thanks for the review. PTAL.

http://codereview.appspot.com/4815061/diff/3006/src/pkg/image/tiff/reader_tes...
File src/pkg/image/tiff/reader_test.go (right):

http://codereview.appspot.com/4815061/diff/3006/src/pkg/image/tiff/reader_tes...
src/pkg/image/tiff/reader_test.go:12: // This test tries to decode an image that
has no RowsPerStrip tag.
On 2011/07/27 02:22:07, nigeltao wrote:
> s/This test/TestNoRPS/.

Done.

http://codereview.appspot.com/4815061/diff/3006/src/pkg/image/tiff/reader_tes...
src/pkg/image/tiff/reader_test.go:16: f, err := os.Open("testdata/rps0.tiff")
On 2011/07/27 02:22:07, nigeltao wrote:
> If the func is called TestNoRPS, then call the test image
> "testdata/no_rps.tiff".

Done.

http://codereview.appspot.com/4815061/diff/3006/src/pkg/image/tiff/reader_tes...
src/pkg/image/tiff/reader_test.go:18: t.Errorf(err.String())
On 2011/07/27 02:22:07, nigeltao wrote:
> t.Error(err)
> return
> 
> Same below.

Done; I used t.Fatal(err) instead.

[nigeltao, 2011-07-28 01:11:35]

LGTM.

[nigeltao, 2011-07-28 01:17:04]

*** Submitted as http://code.google.com/p/go/source/detail?r=854c7c44697c ***

image/tiff: Do not panic when RowsPerStrip is missing.

The RowsPerStrip tag is mandatory according to the spec. However,
Mac OS sometimes (?) omits it. I managed to create such an image
by applying "tiffutil -none" on a compressed image.

If RowsPerStrip is 0, there was a division by zero in the decoder.
Assume that the image is a single strip in this case.

R=nigeltao, bradfitz
CC=golang-dev
http://codereview.appspot.com/4815061

Committer: Nigel Tao <nigeltao@golang.org>