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.

Sign up for GitHub

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

Jump to bottom

x/image: CCITT reader fails to return last row of data #34809

Closed
dchang-dchang opened this issue Oct 10, 2019 · 6 comments
Closed

x/image: CCITT reader fails to return last row of data #34809

dchang-dchang opened this issue Oct 10, 2019 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@dchang-dchang
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.12.8 linux/amd64

Does this issue reproduce with the latest release?

This reproduces on the master branch of x/image.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH=amd64
GOBIN=
GOCACHE=/home/dchang/.cache/go-build
GOEXE=
GOFLAGS=
GOHOSTARCH=amd64
GOHOSTOS=linux
GOOS=linux
GOPATH=/home/dchang/go
GOPROXY=
GORACE=
GOROOT=/usr/local/go
GOTMPDIR=
GOTOOLDIR=/usr/local/go/pkg/tool/linux_amd64
GCCGO=gccgo
CC=gcc
CXX=g++
CGO_ENABLED=1
GOMOD=
CGO_CFLAGS=-g -O2
CGO_CPPFLAGS=
CGO_CXXFLAGS=-g -O2
CGO_FFLAGS=-g -O2
CGO_LDFLAGS=-g -O2
PKG_CONFIG=pkg-config
GOGCCFLAGS=-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/home/dchang/tmp/go-build386659025=/tmp/go-build -gno-record-gcc-switches

What did you do?

package main

import (
	"net/http"

	"golang.org/x/image/tiff"
)

func main() {

	resp, err := http.Get("https://github.com/golang/image/raw/cc248b1a6b0721f0d86b47a99c46fe460b2faac5/testdata/group3_test.tiff")
	if err != nil {
		panic(err)
	}
	defer resp.Body.Close()
	img, err := tiff.Decode(resp.Body)
	if err != nil {
		panic(err)
	}
	fmt.Println(img.Bounds())
}

What did you expect to see?

(0,0)-(1728,1103)

What did you see instead?

panic: tiff: invalid format: not enough pixel data

First, thank you @nigeltao and others for your work on the CCITT Group3 TIFF reader. Really coming in handy right as we're trying to parse TIFF fax files!

Second, let me preface this with the face that I had never heard of CCITT before this morning. I'm likely to have gotten a bunch wrong so for that, thanks for your patience and help!

The TLDR is that I think the CCITT reader is, in certain cases, decoding but not "packing" the last line

It happens when the io.Reader returns n bytes and also a io.EOF error [0]. This causes the CCITT reader to break immediately, and thus doesn't actually pack and thus results in the last row not part of the returned bytes, causing the TIFF package to be confused by the missing data (tiff: invalid format: not enough pixel data).

I found a group3 compressed tiff file from libtiff [1] that seems to exhibit this problem and the code in this PR [2] seems to address it.

Thanks for all the help!

Thanks!
David

[0] https://golang.org/pkg/io/#Reader

When Read encounters an error or end-of-file condition after
successfully reading n > 0 bytes, it returns the number of bytes read.
It may return the (non-nil) error from the same call or return the error
(and n == 0) from a subsequent call.

[1] See http://www.simplesystems.org/libtiff/images.html

[2] Created the PR first, but sounds like the issue tracker is here and so duplicating. Sorry for the noise and feel free to work off which ever one is convenient and we can close the other one.
@gopherbot gopherbot added this to the Unreleased milestone Oct 10, 2019
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 10, 2019
@dchang-dchang dchang-dchang mentioned this issue Oct 15, 2019
Closed
@gopherbot
Copy link

Change https://golang.org/cl/200162 mentions this issue: ccitt: return all data from CCITT reader

@nigeltao
Copy link
Contributor

First, thank you @nigeltao and others

The others, @hhrutter and @bsiegert deserve more thanks than me.

decoding but not "packing" the last line

This is possibly related to https://go-review.googlesource.com/c/image/+/198547 which @hhrutter has coincidentally also sent out recently.

As for this issue (and CL 200162), perhaps @hhrutter and @bsiegert can take a first look at this and how it relates to CL 198547. I'm busier than usual with other things for the next few weeks.

@hhrutter
Copy link

hhrutter commented Oct 18, 2019 •
edited

This issue should be taken care of by CL 201938.
Note: The mentioned CL is tentative.

@dchang-dchang just for verification, please state which file of libtiff/images was giving you problems.

@dchang-dchang
Copy link
Author

Great! Totally agree that Cl 201938 will address this.

g3test.tif is the file that I was using. Thanks!

@gopherbot
Copy link

Change https://golang.org/cl/201938 mentions this issue: ccitt: suggested reader.go changes

@gopherbot
Copy link

Change https://golang.org/cl/211037 mentions this issue: ccitt: accept missing multiple-EOL trailer

@golang golang locked and limited conversation to collaborators Dec 13, 2020
GalaxyForcew added a commit to GalaxyForcew/A1bisshy that referenced this issue Jun 6, 2022
@GalaxyForcew
ccitt: accept missing multiple-EOL trailer
39345b1 
The new bw-gopher-truncatedX.ccitt_groupY files were derived from the
existing bw-gopher.ccitt_groupY files, after dropping the final 6
consecutive EOL's or 2 consecutive EOL's (depending on whether Y is 3 or
4) and then padding to a byte boundary with either all 0 bits or all 1
bits (depending on X). Each EOL code is 12 bits long: 0000_0000_0001.

Fixes golang/go#34809

Change-Id: Ibb2d964b5205b28f5e2adb5d30647b92aec53c77
Reviewed-on: https://go-review.googlesource.com/c/image/+/211037
Reviewed-by: Benny Siegert <bsiegert@gmail.com>
Reviewed-by: Horst Rutter <hhrutter@gmail.com>
Run-TryBot: Benny Siegert <bsiegert@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
yi-ge3 added a commit to yi-ge3/wislie that referenced this issue Jun 6, 2022
@yi-ge3
ccitt: accept missing multiple-EOL trailer
206691c 
The new bw-gopher-truncatedX.ccitt_groupY files were derived from the
existing bw-gopher.ccitt_groupY files, after dropping the final 6
consecutive EOL's or 2 consecutive EOL's (depending on whether Y is 3 or
4) and then padding to a byte boundary with either all 0 bits or all 1
bits (depending on X). Each EOL code is 12 bits long: 0000_0000_0001.

Fixes golang/go#34809

Change-Id: Ibb2d964b5205b28f5e2adb5d30647b92aec53c77
Reviewed-on: https://go-review.googlesource.com/c/image/+/211037
Reviewed-by: Benny Siegert <bsiegert@gmail.com>
Reviewed-by: Horst Rutter <hhrutter@gmail.com>
Run-TryBot: Benny Siegert <bsiegert@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
snapbakkhfbav added a commit to snapbakkhfbav/SayedBaladohr that referenced this issue Oct 6, 2022
@snapbakkhfbav
ccitt: accept missing multiple-EOL trailer
3612268 
The new bw-gopher-truncatedX.ccitt_groupY files were derived from the
existing bw-gopher.ccitt_groupY files, after dropping the final 6
consecutive EOL's or 2 consecutive EOL's (depending on whether Y is 3 or
4) and then padding to a byte boundary with either all 0 bits or all 1
bits (depending on X). Each EOL code is 12 bits long: 0000_0000_0001.

Fixes golang/go#34809

Change-Id: Ibb2d964b5205b28f5e2adb5d30647b92aec53c77
Reviewed-on: https://go-review.googlesource.com/c/image/+/211037
Reviewed-by: Benny Siegert <bsiegert@gmail.com>
Reviewed-by: Horst Rutter <hhrutter@gmail.com>
Run-TryBot: Benny Siegert <bsiegert@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

Successfully merging a pull request may close this issue.

x/image/ccitt: return all data from CCITT reader
5 participants
@ALTree @nigeltao @gopherbot @hhrutter @dchang-dchang