Code review - Issue 4815061: code review 4815061: image/tiff: Do not panic when RowsPerStrip is missing.https://codereview.appspot.com/2011-07-28T01:17:04+00:00rietveld
Message from unknown
2011-07-26T17:49:51+00:00bsiegerturn:md5:bccc64e36e11fd9e3e16316e18d9df8e
Message from unknown
2011-07-26T17:50:04+00:00bsiegerturn:md5:27123520d0e25b49b2f1e917269fc759
Message from unknown
2011-07-26T17:50:39+00:00bsiegerturn:md5:87ee66291808cc25eabbc42fd0956c21
Message from bsiegert@gmail.com
2011-07-26T17:50:43+00:00bsiegerturn:md5:8fcd205c701aced9c349e6612e1b850b
Hello nigeltao@golang.org (cc: golang-dev@googlegroups.com),
I'd like you to review this change to
https://go.googlecode.com/hg/
Message from bradfitz@golang.org
2011-07-26T17:54:30+00:00bradfitzurn:md5:1f61314dda2d49375f426576f6f3a538
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")
>
>
>
Message from bsiegert@gmail.com
2011-07-26T18:14:13+00:00bsiegerturn:md5:811504bda884bdea3c40782732d0d94b
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)
Message from unknown
2011-07-26T19:40:23+00:00bsiegerturn:md5:204335a853e01fe3a07a3ac39c47c8cc
Message from bsiegert@gmail.com
2011-07-26T19:40:27+00:00bsiegerturn:md5:eb89b48e3243422cf534f165e1aabb99
Hello nigeltao@golang.org, bradfitz@golang.org (cc: golang-dev@googlegroups.com),
Please take another look.
Message from unknown
2011-07-26T19:41:18+00:00bsiegerturn:md5:a3ee6f81ca518c769df0e91f82cc27df
Message from unknown
2011-07-26T19:44:12+00:00bsiegerturn:md5:1132d70842095ab106a08cc437fe3e9e
Message from nigeltao@golang.org
2011-07-27T02:22:07+00:00nigeltaourn:md5:d42dc47f4d36dc102dcc2007a0cd30d0
http://codereview.appspot.com/4815061/diff/3006/src/pkg/image/tiff/reader_test.go
File src/pkg/image/tiff/reader_test.go (right):
http://codereview.appspot.com/4815061/diff/3006/src/pkg/image/tiff/reader_test.go#newcode12
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_test.go#newcode16
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_test.go#newcode18
src/pkg/image/tiff/reader_test.go:18: t.Errorf(err.String())
t.Error(err)
return
Same below.
Message from unknown
2011-07-27T18:55:02+00:00bsiegerturn:md5:45b1544d1c3e5e0e03182f8c35aabef8
Message from bsiegert@gmail.com
2011-07-27T18:55:12+00:00bsiegerturn:md5:0d60531e621e2a4fe674cbbf0cbc9110
Thanks for the review. PTAL.
http://codereview.appspot.com/4815061/diff/3006/src/pkg/image/tiff/reader_test.go
File src/pkg/image/tiff/reader_test.go (right):
http://codereview.appspot.com/4815061/diff/3006/src/pkg/image/tiff/reader_test.go#newcode12
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_test.go#newcode16
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_test.go#newcode18
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.
Message from nigeltao@golang.org
2011-07-28T01:11:35+00:00nigeltaourn:md5:490c51fc512fb7f738f15679b21d14e6
LGTM.
Message from nigeltao@golang.org
2011-07-28T01:17:04+00:00nigeltaourn:md5:b5a7ff8908023ae7a031b3548650e65a
*** 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>