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

compress/gzip: index out of range panic when closing a gzip.Writer #3998

Closed
gopherbot opened this issue Aug 23, 2012 · 22 comments
Closed

compress/gzip: index out of range panic when closing a gzip.Writer #3998

gopherbot opened this issue Aug 23, 2012 · 22 comments

Comments

@gopherbot
Copy link

by tav@espians.com:

What steps will reproduce the problem?

I've not been able to reproduce the problem, but during an HTTP request, I write to a
gzip.Writer backed by a bytes.Buffer and when I call Close() on the gzip.Writer, it
throws an index out of range panic.

What is the expected output?

The gzip.Writer should close and flush to the underlying writer.

What do you see instead?

An index out of range panic:

/Users/tav/silo/ampify/environ/local/go/src/pkg/net/http/server.go:593 (0x7a093)
    func.004: buf.Write(debug.Stack())
/Users/tav/silo/ampify/environ/local/go/src/pkg/runtime/panic.c:134 (0x1b9e6)
    panic: reflect·call(d->fn, (byte*)d->args, d->siz);
/Users/tav/silo/ampify/environ/local/go/src/pkg/runtime/panic.c:384 (0x1c0d3)
    panicstring: runtime·panic(err);
/Users/tav/silo/ampify/environ/local/go/src/pkg/runtime/panic.c:341 (0x1bf65)
    panicindex: runtime·panicstring("index out of range");
/Users/tav/silo/ampify/environ/local/go/src/pkg/compress/flate/deflate.go:177 (0x1040f8)
    (*compressor).findMatch: if w0 == win[i] && w1 == win[i+1] && wEnd == win[i+length] {
/Users/tav/silo/ampify/environ/local/go/src/pkg/compress/flate/deflate.go:286 (0x104796)
    (*compressor).deflate: if newLength, newOffset, ok := d.findMatch(d.index, d.chainHead-d.hashOffset, minMatchLength-1, lookahead); ok {
/Users/tav/silo/ampify/environ/local/go/src/pkg/compress/flate/deflate.go:421 (0x10543d)
    (*compressor).close: d.step(d)
/Users/tav/silo/ampify/environ/local/go/src/pkg/compress/flate/deflate.go:507 (0x1056a5)
    (*Writer).Close: return w.d.close()
/Users/tav/silo/ampify/environ/local/go/src/pkg/compress/gzip/gzip.go:209 (0x400ca)
    (*Writer).Close: z.err = z.compressor.Close()
/Users/tav/silo/planfile-app/planfile.go:693 (0xb9c0)
    func.002: enc.Close()
/Users/tav/silo/ampify/environ/local/go/src/pkg/net/http/server.go:721 (0x6fb4d)
    HandlerFunc.ServeHTTP: f(w, r)
/Users/tav/silo/ampify/environ/local/go/src/pkg/net/http/server.go:962 (0x70a29)
    (*ServeMux).ServeHTTP: mux.handler(r).ServeHTTP(w, r)
/Users/tav/silo/ampify/environ/local/go/src/pkg/net/http/server.go:673 (0x6f93c)
    (*conn).serve: handler.ServeHTTP(w, w.req)
/Users/tav/silo/ampify/environ/local/go/src/pkg/runtime/proc.c:270 (0x1d550)
    goexit: runtime·goexit(void)

Which compiler are you using (5g, 6g, 8g, gccgo)?

go (6g)

Which operating system are you using?

OS X 10.7.4

Which version are you using?  (run 'go version')

go version weekly.2012-03-27 +547c731ab129
@rsc
Copy link
Contributor

rsc commented Aug 31, 2012

Comment 1:

Labels changed: added priority-later, removed priority-triage.

Status changed to Accepted.

@krasin
Copy link

krasin commented Aug 31, 2012

Comment 2:

Hi Tav,
thank you for reporting this issue.
The first step for me, in order to fix this bug, would be to make a reproducer.
Unlike the most of other (already fixed) bugs in compress/deflate, this one looks to be
hard. 
Could you please try to answer the following questions? The answers (even incomplete)
will help me a lot.
1. Is this a one-time bug or you spot it regularly?
(I hope it's not one-timer, because in this case, I will have a very nice puzzle to
solve)
Below, I assume that this bug was spotted at least few times and there's no reason to
believe that it won't crash your program again.
2. What's is the period between these crashes? A minute/hour/day/week?
3. What was the compression level?
4. What's approximate data input size?
5. Is it possible to record the data which leads to the crash?
6. Is it possible to use a modified Go runtime (if provided by Go team) to compile your
program and have more detailed crash report in the next time when this bug appeared?

@krasin
Copy link

krasin commented Sep 4, 2012

Comment 3:

an update. It appears that the code is open-sourced:
https://github.com/tav/planfile-app/blob/master/planfile.go#L693
i have not yet looked into it closely, though.

@krasin
Copy link

krasin commented Sep 4, 2012

Comment 4:

Starting to self-answer to the questions above.
Q3: What was the compression level?
A3: DefaultCompression (i.e. 6)

@krasin
Copy link

krasin commented Sep 4, 2012

Comment 5:

http://golang.org/src/pkg/compress/flate/deflate.go#L228
I suspect that the condition to raise this bug is:
d.sync = true
d.index >= d.maxInsertIndex
lookahead != 0 (i.e. lookahead == 1 or lookeahead == 2)
+ some luck
I will try to exploit this tomorrow. Even if I would not succeed, it probably makes
sense to handle this situation more explicitly at
http://golang.org/src/pkg/compress/flate/deflate.go#L251

@krasin
Copy link

krasin commented Sep 4, 2012

Comment 6:

See also http://golang.org/cl/6496079
This is a possible fix to this bug. I will not send it to the actual review, until I
found a test case that triggers the issue.

@gopherbot
Copy link
Author

Comment 7 by tav@espians.com:

Hey Ivan,
Thanks for looking into this and I'm impressed that you managed to find the planfile
source! To answer your various questions:
1. It's not regular, and I've only seen it once — but Seyi, a fellow hacker, has
independently experienced it too:
https://raw.github.com/gist/be2ae5a6b629ba4ebb48/9278242906f2b9f8f516e4dd90ba8f5e3e1554a9/error.log
2. Only experienced the crash once each so far. But, having said that, we've not been
running the code in production yet.
4. We've not isolated it down to an individual request yet. Request data sizes currently
vary all the way up to around 21kb uncompressed.
5. Yes, I can add code to log the requests.
6. Yes, I can compile/run modified Go runtimes.

@krasin
Copy link

krasin commented Sep 4, 2012

Comment 8:

Hi Tav,
thank you for your answers. Currently, I have very high expectations to find a
reproducer tomorrow (after a good sleep). If I failed (which would not surprise me), I
would be happy to provide a patch to Go runtime and/or a patch to planfile.
(semi-unrelated) It would be really nice to have an introduction to planfile in
README.md and a short instruction on how to get started. I have spent few minutes on the
idea to run local tests with planfile-app, but I have given up on reading main():
https://github.com/tav/planfile-app/blob/master/planfile.go#L611

@krasin
Copy link

krasin commented Sep 6, 2012

Comment 9:

A non-update. I am still trying to find a reproducer.
I have tried to read the code & think as well as I have ran the tests on all
significantly-different sequences of the length 14 and smaller with no crash.
I have not given up yet. When I did, I would make a patch to
src/pkg/compress/deflate/deflate.go with a better diagnostics.

@krasin
Copy link

krasin commented Sep 6, 2012

Comment 10:

Hi Tav,
I was unable to find a reproducer. Any analysis ended up with "this can't happen,
because it can't happen ever" exclamation. This likely means that I miss something
obvious.
Anyway, I want to proceed to the plan B: improve diagnostics to be prepared to the next
time. What do you prefer: a modified compress/flate or a modified planfile.go?
A modified Go runtime has a drawback that it's unlikely to be used by anybody else than
you and given the low rate of this crash, all odds that the next time it would fail not
on your machine.
I am fine to provide a patch to any of these components.
If you choose to patch planfile, I would really appreciate an instruction how to build
it, because it seems that planfile is not Go installable:
$ go get github.com/tav/planfile-app
package github.com/tav/planfile-app
    imports amp/crypto: unrecognized import path "amp/crypto"
package github.com/tav/planfile-app
    imports amp/httputil: unrecognized import path "amp/httputil"
package github.com/tav/planfile-app
    imports amp/log: unrecognized import path "amp/log"
package github.com/tav/planfile-app
    imports amp/oauth: unrecognized import path "amp/oauth"
package github.com/tav/planfile-app
    imports amp/optparse: unrecognized import path "amp/optparse"
package github.com/tav/planfile-app
    imports amp/runtime: unrecognized import path "amp/runtime"
package github.com/tav/planfile-app
    imports amp/tlsconf: unrecognized import path "amp/tlsconf"

@gopherbot
Copy link
Author

Comment 11 by micrypt:

Hi Ivan,
I've updated the Planfile readme
(https://github.com/tav/planfile-app/blob/master/README.md) with some instructions on
getting it running. 
Thanks,
Seyi

@krasin
Copy link

krasin commented Sep 12, 2012

Comment 12:

a non-update. I am temporary extremely busy on something else. I will be back to work on
this issue next week. Sorry for a delay.
Have you seen this issue ever again, or it's really rare and you still saw it twice, as
discussed earlier in this thread?

@rsc
Copy link
Contributor

rsc commented Sep 12, 2012

Comment 13:

Labels changed: added go1.1maybe.

@gopherbot
Copy link
Author

Comment 14 by tav@espians.com:

Hey Ivan,
I'd like to close this issue for now. In over a month of running the code in production,
I've not been able to run into the same bug again... :(

@krasin
Copy link

krasin commented Oct 29, 2012

Comment 15:

Hi Tav,
let's keep it open for some time. I have not forgot about it and still keep an "unread"
email about this issue in my personal e-mail. It just goes that I still don't have time
for a serious attack for this issue due to the rush on my primary job.
In short, before closing this bug with the words "not reproducible", I would like to run
compressor on a large set of text documents and see how this will work.
Sorry for being so unresponsive so far.

@rsc
Copy link
Contributor

rsc commented Dec 9, 2012

Comment 16:

Leaving open but not for Go 1.1.
We might just replace the compressor anyway.

Labels changed: removed go1.1maybe.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 17:

Labels changed: added go1.2.

@robpike
Copy link
Contributor

robpike commented Aug 21, 2013

Comment 18:

Removing Go 1.2 tag.

Labels changed: removed go1.2.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 19:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 20:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 21:

Labels changed: added repo-main.

@ianlancetaylor
Copy link
Contributor

Comment 22:

Labels changed: removed priority-later.

Status changed to TimedOut.

@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants