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
Labels
Comments
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? |
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. |
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 |
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. |
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. |
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 |
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. |
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" |
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 |
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... :( |
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. |
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
by tav@espians.com:
The text was updated successfully, but these errors were encountered: