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

Open
astromechza opened this issue Nov 10, 2018 · 4 comments · May be fixed by astromechza/go#1
Milestone

Comments

@astromechza
Copy link

@astromechza astromechza commented Nov 10, 2018

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

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented 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

This comment has been minimized.

Copy link
Author

@astromechza astromechza commented Nov 10, 2018

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

This comment has been minimized.

Copy link

@borud borud commented Aug 12, 2019

Any chance this will be fixed in 1.12?

@nigeltao

This comment has been minimized.

Copy link
Contributor

@nigeltao nigeltao commented Aug 14, 2019

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.