Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/image/tiff: index out of range #11386

Open
dvyukov opened this issue Jun 24, 2015 · 11 comments
Open

x/image/tiff: index out of range #11386

dvyukov opened this issue Jun 24, 2015 · 11 comments
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@dvyukov
Copy link
Member

dvyukov commented Jun 24, 2015

The following program:

package main

import (
    "bytes"
    "golang.org/x/image/tiff"
    "io/ioutil"
    "os"
)

func main() {
    data, _ := ioutil.ReadFile(os.Args[1])
    tiff.Decode(bytes.NewReader(data))
}

on this file:
https://drive.google.com/file/d/0B20Uwp8Hs1oCdDhyRzAwbE5qc2M/view?usp=sharing
crashes as follows:

panic: runtime error: index out of range

goroutine 1 [running]:
io/ioutil.readAll.func1(0xc8200436c0)
    src/io/ioutil/ioutil.go:30 +0x11e
golang.org/x/image/tiff/lzw.(*decoder).decode(0xc8200b1300)
    src/golang.org/x/image/tiff/lzw/reader.go:187 +0x75b
golang.org/x/image/tiff/lzw.(*decoder).Read(0xc8200b1300, 0xc8200cb001, 0xdff, 0xdff, 0x201, 0x0, 0x0)
    src/golang.org/x/image/tiff/lzw/reader.go:141 +0x19e
bytes.(*Buffer).ReadFrom(0xc820043618, 0x7f23a2e99518, 0xc8200b1300, 0x1001, 0x0, 0x0)
    src/bytes/buffer.go:173 +0x23f
io/ioutil.readAll(0x7f23a2e99518, 0xc8200b1300, 0x200, 0x0, 0x0, 0x0, 0x0, 0x0)
    src/io/ioutil/ioutil.go:33 +0x154
io/ioutil.ReadAll(0x7f23a2e99518, 0xc8200b1300, 0x0, 0x0, 0x0, 0x0, 0x0)
    src/io/ioutil/ioutil.go:42 +0x51
golang.org/x/image/tiff.Decode(0x7f23a2e992d8, 0xc820018420, 0x7f23a2e99438, 0xc820012480, 0x0, 0x0)
    src/golang.org/x/image/tiff/reader.go:646 +0xf2a
main.main()
    tiff.go:12 +0xf2

on commit eb11b45157c1b71f30b3cec66306f1cd779a689e
go version devel +3cab476 Sun Jun 21 03:11:01 2015 +0000 linux/amd64

@ianlancetaylor ianlancetaylor added this to the Unreleased milestone Jul 11, 2015
@hhrutter
Copy link

This file looks corrupt to me.
The Mac Preview also has the same rendering problem.
I attached a screenshow of what the Preview looks like:

screen shot 2018-10-27 at 15 24 22

This Tiff has 6 tiles (150 x 100) and decoding chokes on the 3rd tile with x/image/tiff.
This looks consistent with what Mac Preview renders.

The actual reason: corrupt lzw data

@bcmills
Copy link
Contributor

bcmills commented Aug 19, 2019

@hhrutter, if the file is corrupt, the tiff package should return an error, not provoke a panic.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 19, 2019
@gregory-m
Copy link
Contributor

Decoding this image first hits this line:
https://github.com/golang/image/blob/cff245a6509b8c4de022d0d5b9037c503c5989d6/tiff/lzw/reader.go#L213

And next code decoding hits this:
https://github.com/golang/image/blob/cff245a6509b8c4de022d0d5b9037c503c5989d6/tiff/lzw/reader.go#L187

Using decoderInvalidCode = 0xffff as c and d.prefix length is only 4096.

I trying to create simple test without adding corrupted file to repo, but this is complicated case and I have not succeeded yet.

Maybe in this case is ok to add file to repo?

P.S.
I think compress/lzw is also affected.

@hhrutter
Copy link

@bsiegert may be able to give some input.
he is one of the maintainers of x/image/tiff

@drswork
Copy link

drswork commented Aug 21, 2019

Having corrupted image files as part of the test suite seems sensible, if for no other reason than to test the error paths.

@nigeltao
Copy link
Contributor

I think compress/lzw is also affected.

I don't think that compress/lzw in the stdlib is also affected. The golang.org/x/image/tiff/lzw variant has this line:

if d.hi+1 >= d.overflow { // NOTE: the "+1" is where TIFF's LZW differs from the standard algorithm.

and, crucially, the +1 isn't in the stdlib's compress/lzw. In the stdlib variant, we don't hit the d.last = decoderInvalidCode line until d.hi hits 0x1000, not 0x0FFF in the x/image variant. Furthermore, the code variable is masked with (1<<d.width - 1), which maxes out at 0x0FFF. So, in the stdlib, we shouldn't get into the if code == d.hi block with a bad (out of bounds) d.last...

I'm still thinking about how best to fix the x/image/variant. To repeat what bcmills@ said, we should definitely return an error, not panic.

@nigeltao
Copy link
Contributor

Ah, the stdlib's compress/lzw/reader.go and the x lib's golang.org/x/image/tiff/lzw/reader.go are meant to be identical (bar a couple of explicitly called out lines), but there were a couple of 2017 era patches to the stdlib that I forgot to also apply to the x lib:

https://go-review.googlesource.com/c/go/+/42032/
https://go-review.googlesource.com/c/go/+/45111/

The second of these (45111) is trivial to copy across, and should fix the immediate problem. The first of these (42032) should also be copied across at some point, but it might need a little more thinking.

@gopherbot
Copy link

Change https://golang.org/cl/191221 mentions this issue: tiff/lzw: don't follow code == hi if last is invalid

@nigeltao
Copy link
Contributor

I don't think that compress/lzw in the stdlib is also affected.

As for why the stdlib version now needs an explicit d.last != decoderInvalidCode check, even though I argued an hour ago that it didn't need such a check... well, it used to not need such a check, but the 42032 changed introduced the need, due to a new d.hi-- line of code.

Anyway, as I said, 42032 should also be copied across, but it needs a little more thinking.

@gregory-m
Copy link
Contributor

I have a question. If stdlib and x/iamge version should be the same except +1 thing, and we struggling to apply patches to both versions, why we can't remove x/image version and add something like NewTIFFReader() and one if to stdlib version? The performance penalty of one if is not that big to justify support of 2 different versions.

gopherbot pushed a commit to golang/image that referenced this issue Aug 23, 2019
This does for x/image what
https://go-review.googlesource.com/c/go/+/45111/ did for the standard
library's compress/lzw.

The x variant is a fork of the stdlib, with an extra, explicit tweak
because the TIFF format is "off by one" - a mistake (not Go specific)
somebody introduced decades ago and that we can never fix, given all the
existing TIFF files out there in the wild.

When previously patching the stdlib variant, I was supposed to also
patch the x variant, but forgot.

Updates golang/go#11386

Change-Id: Ic74f9014d2d048ee12cdd151332db62b76f1cde2
Reviewed-on: https://go-review.googlesource.com/c/image/+/191221
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@nigeltao
Copy link
Contributor

why we can't remove x/image version and add something like NewTIFFReader() and one if to stdlib version? The performance penalty of one if is not that big to justify support of 2 different versions.

The performance penalty would have to be measured. It's not just one "if", it's one "if" in the inner loop.

But in general, putting TIFF's off-by-one into the stdlib's compress/lzw is discussed in #25409

GalaxyForcew added a commit to GalaxyForcew/A1bisshy that referenced this issue Jun 6, 2022
This does for x/image what
https://go-review.googlesource.com/c/go/+/45111/ did for the standard
library's compress/lzw.

The x variant is a fork of the stdlib, with an extra, explicit tweak
because the TIFF format is "off by one" - a mistake (not Go specific)
somebody introduced decades ago and that we can never fix, given all the
existing TIFF files out there in the wild.

When previously patching the stdlib variant, I was supposed to also
patch the x variant, but forgot.

Updates golang/go#11386

Change-Id: Ic74f9014d2d048ee12cdd151332db62b76f1cde2
Reviewed-on: https://go-review.googlesource.com/c/image/+/191221
Reviewed-by: Bryan C. Mills <bcmills@google.com>
yi-ge3 added a commit to yi-ge3/wislie that referenced this issue Jun 6, 2022
This does for x/image what
https://go-review.googlesource.com/c/go/+/45111/ did for the standard
library's compress/lzw.

The x variant is a fork of the stdlib, with an extra, explicit tweak
because the TIFF format is "off by one" - a mistake (not Go specific)
somebody introduced decades ago and that we can never fix, given all the
existing TIFF files out there in the wild.

When previously patching the stdlib variant, I was supposed to also
patch the x variant, but forgot.

Updates golang/go#11386

Change-Id: Ic74f9014d2d048ee12cdd151332db62b76f1cde2
Reviewed-on: https://go-review.googlesource.com/c/image/+/191221
Reviewed-by: Bryan C. Mills <bcmills@google.com>
snapbakkhfbav added a commit to snapbakkhfbav/SayedBaladohr that referenced this issue Oct 6, 2022
This does for x/image what
https://go-review.googlesource.com/c/go/+/45111/ did for the standard
library's compress/lzw.

The x variant is a fork of the stdlib, with an extra, explicit tweak
because the TIFF format is "off by one" - a mistake (not Go specific)
somebody introduced decades ago and that we can never fix, given all the
existing TIFF files out there in the wild.

When previously patching the stdlib variant, I was supposed to also
patch the x variant, but forgot.

Updates golang/go#11386

Change-Id: Ic74f9014d2d048ee12cdd151332db62b76f1cde2
Reviewed-on: https://go-review.googlesource.com/c/image/+/191221
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

8 participants