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

image/jpeg: bad RST marker due to pre-reset marker byte alignment #28717

Closed
astromechza opened this issue Nov 10, 2018 · 6 comments
Closed

image/jpeg: bad RST marker due to pre-reset marker byte alignment #28717

astromechza opened this issue Nov 10, 2018 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@astromechza
Copy link

What did you do?

While writing a small app to decode and process the JPEG frames from webcams running in Motion-JPEG mode, I found that images from a Logitech C270 webcam failed to decode when using the jpeg.Decode function. Images from other /dev/video devices worked just fine. I confirmed that the same images decoded fine using other programs like vlc.

I isolated a frame that caused the decoder to fail and stepped through the decoding with a debugger and compared it to the part of the jpeg spec in F1.2.3 from https://www.w3.org/Graphics/JPEG/itu-t81.pdf#page=36&zoom=auto,-200,43.

Turns out the jpeg decoder doesn't handle 0xFF 0x00 bytes that precede the expected 0xFF 0xD* bytes that form the reset markers. The stuffed bytes are used for byte alignment.

Here's an example frame from the stream:

fail

And here's a play.golang.org link with a reproducer: https://play.golang.org/p/QTTKiHRfrLe

I've experimented with a fix in the handling of the rst marker in the image/jpeg/scan.go file and I'm fairly confident that this should fix the issue (it certainly seemed to fix it in my usecase):

diff image/jpeg/scan.go /snap/go/current/src/image/jpeg/scan.go 
313,320d312
< 
<                               // detect the presence of a stuffed 0xff00 pair caused by byte alignment
<                               if d.tmp[0] == 0xff && d.tmp[1] == 0x00 {
<                                       if err := d.readFull(d.tmp[:2]); err != nil {
<                                               return err
<                                       }
<                               }
< 

What did you expect to see?

Expected the frame to decode successfully as it is in other software like VLC and web browsers.

What did you see instead?

panic: invalid JPEG format: bad RST marker

Does this issue reproduce with the latest release (go1.11.2)?

Yes.

System details

go version go1.11 linux/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ben/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ben/.gvm/pkgsets/go1.11/global"
GOPROXY=""
GORAwE=""
GOROOT="/home/ben/.gvm/gos/go1.11"
GOTMPDIR=""
GOTOOLDIR="/home/ben/.gvm/gos/go1.11/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/ben/projects/github.com/AstromechZA/scream/go.mod"
GOROOT/bin/go version: go version go1.11 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.11
uname -sr: Linux 4.15.0-36-generic
Distributor ID:	Ubuntu
Description:	Ubuntu 18.04.1 LTS
Release:	18.04
Codename:	bionic
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Ubuntu GLIBC 2.27-3ubuntu1) stable release version 2.27.
gdb --version: GNU gdb (Ubuntu 8.1-0ubuntu3) 8.1.0.20180409-git
@astromechza astromechza changed the title image/jpeg: fails on stuffed bytes due to pre-reset marker byte alignment image/jpeg: bad RST marker due to pre-reset marker byte alignment Nov 10, 2018
@ianlancetaylor
Copy link
Contributor

CC @nigeltao

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 10, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone Nov 10, 2018
astromechza added a commit to astromechza/go that referenced this issue Nov 10, 2018
JPEG standard allows for stuffed bytes just before the reset markers in order to align bytes (refer to B 1, D 1.6, and F 1.2.3 of JPEG spec). Some JPEG encoders seem to use these even when byte alignment is not strictly necessary. This fix checks for and skips over the escaped stuffed byte.

Fixes golang#28717
@astromechza
Copy link
Author

I've setup a branch with the proposed fix on my fork of golang/go. I may be completely wrong about this bug (there's a chance the webcam has a bad jpeg encoder) or there may be a better way to fix the issue 😄

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@borud
Copy link

borud commented Aug 12, 2019

Any chance this will be fixed in 1.12?

@nigeltao
Copy link
Contributor

The patch in the OP looks plausible (although I'd like the comment to mention F1.2.3 in the spec), but we are deep in the release cycle (https://github.com/golang/go/wiki/Go-Release-Cycle) for Go 1.13. As it is not a regression, it will probably land in 1.14 at the earliest.

Sorry for the late reply. I can't remember why I didn't see the "CC @nigeltao" note earlier.

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gunnsth
Copy link

gunnsth commented Apr 8, 2020

Is this anticipated in 1.15?

@gopherbot
Copy link

Change https://golang.org/cl/230122 mentions this issue: image/jpeg: accept "\xff\x00" before a RST marker

xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
Fixes golang#28717

Change-Id: I0a1e4ef1583fff89b6f46ef647fb6e4499bdf999
Reviewed-on: https://go-review.googlesource.com/c/go/+/230122
Run-TryBot: Nigel Tao <nigeltao@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
@golang golang locked and limited conversation to collaborators Apr 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants