kothar / brotli-go

Not maintained -> see itch.io's more up-to-date fork 👉
https://github.com/itchio/go-brotli
MIT License
82 stars 9 forks source link

assertion failed in WriteBits #1

Closed fasterthanlime closed 8 years ago

fasterthanlime commented 8 years ago

Hey, not sure if this is an issue in google/brotli, this, or the way I've used it but I was wondering if you could give a look at https://github.com/itchio/butler/issues/3

You can browse the v0.4.5 tree at https://github.com/itchio/butler/tree/v0.4.5 — the testBrotli function lives at https://github.com/itchio/butler/blob/v0.4.5/butler.go#L151

Basically you should just be able to feed butler.exe (the v0.4.5 windows-amd64 build, which you can grab from this .7z archive) to a compress/decompress test & be able to reproduce.

I'll try to come up with a stack trace & a better/more self-contained test case later on, but if you've already encountered that / have any idea where it might come from, I'd be glad to know about it. Thanks!

kothar commented 8 years ago

I can't reproduce it myself, possibly because I'm developing on a Mac. I've added a very basic compression/decompression implementation, for which the following works for me:

go install gopkg.in/kothar/brotli-go.v0/gbr
gbr -c butler.exe -q 4

From your log, it looks like that should fail on your system - can you check if it does? If not then we should look for differences in the two test cases, and I will try to replicate it on a Windows system.

I tried passing a length-1 buffer for output as in your test, but that worked fine for me too. Note that you can also pass a nil (0-length) output buffer with the same effect.

fasterthanlime commented 8 years ago

Thanks a lot for taking interest in this!

I installed gbr and it does fail too. The current test matrix looks like:

program arch cross-compiled mingw-gcc version crashes?
butler 386 yes 4.6 no
butler x86_64 yes 4.6 yes
gbr x86_64 no 5.2 yes

So whatever the problem is, it seems to be only on 64-bit, affects mingw from versions at least 4.6 to 5.2, whether it's cross-compiled or not, it reliably crashes when compressing butler.exe with q=4.

I checked, q=5 to q=11 all run without crashing.

I'll try compiling with DEBUG_BIT_WRITER and post a log of that.

fasterthanlime commented 8 years ago

Something I forgot to mention: to get it to compile on windows, I have to use these ldflags:

go get -ldflags="-extldflags=-Wl,--allow-multiple-definition" gopkg.in/kothar/brotli-go.v0/gbr

Although these are needed on 32-bit as well, where compressing the same file at q=4 doesn't crash

kothar commented 8 years ago

Thanks for the flags, I think I should be able to add them to the cgo settings for windows which would save some complications if those are always required.

--  Mike Houston @kotharx

On 12 November 2015 at 12:24:41, Amos Wenger (notifications@github.com) wrote:

Something I forgot to mention: to get it to compile on windows, I have to use these ldflags:

go get -ldflags="-extldflags=-Wl,--allow-multiple-definition" gopkg.in/kothar/brotli-go.v0/gbr

Although these are needed on 32-bit as well, where compressing the same file at q=4 doesn't crash

— Reply to this email directly or view it on GitHub.

fasterthanlime commented 8 years ago

This is what happens when one does not use those ldflags:

amos at bowser in ~/tmp/brotli (master)
$ go install gopkg.in/kothar/brotli-go.v0/gbr
# gopkg.in/kothar/brotli-go.v0/gbr
C:\Go\pkg\tool\windows_amd64\link.exe: running g++ failed: exit status 1
C:\msys64\tmp\go-link-991828467/000001.o:mingw_helpers.c:(.text+0x14340): multiple definition of `___chkstk_ms'
C:\msys64\tmp\go-link-991828467/000000.o:streams.c:(.text+0x6d30): first defined here
collect2.exe: error: ld returned 1 exit status

Those flags are an ugly workaround btw, go folks consider it a bug in cgo they want to resolve another way, cf. https://github.com/golang/go/issues/9510 and https://golang.org/cl/16741

I'm wondering if these flags could cause the WriteBits issue.. but I don't really see how. I'll keep digging. Currently trying to compile google/brotli to see if the go wrapper is involved in the issue at all.

fasterthanlime commented 8 years ago

First few experiments with google/brotli at a08855c715f2060c515acb478d44684e54deef9b: both 32-bit and 64-bit versions of tools/bro.exe built with mingw seem to choke on a few of their tests files (prints corrupted input while trying to decompress)

It doesn't seem to be about optimization flags either, passing -O0 doesn't help at all.

fasterthanlime commented 8 years ago

@kothar is it easy for you to tell which revision of google/brotli is vendored in brotli-go ? I'm curious. I'm git-bisecting through their tree and nothing works with mingw so far (so, brotli-go works better than upstream for now..)

fasterthanlime commented 8 years ago

Switching to https://github.com/Bulat-Ziganshin/brotli/commit/e87bece2372d787b25ebed7df550699ee879eaca allows me to build a 64-bit bro.exe that can run their whole roundtrip test suite

fasterthanlime commented 8 years ago

Alright, bro.exe sucessfully compresses butler.exe at q=4, so now I'm actually suspecting brotli-go and/or the ldflags I had to pass for it to compile:

amos at bowser in ~/tmp
$ gbr -c butler.exe -q 4

This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information.
Assertion failed!

Program: C:\msys64\home\amos\Dev\go\bin\gbr.exe
File: C:/msys64/home/amos/Dev/go/src/gopkg.in/kothar/brotli-go.v0/enc/write_bits.h, Line 52

Expression: bits < 1UL << n_bits

amos at bowser in ~/tmp
$ brotli/tools/bro.exe -q 4 -i butler.exe >! butler.exe.bro

amos at bowser in ~/tmp
$ gbr -d butler.exe.bro

amos at bowser in ~/tmp
$ brotli/tools/bro.exe -d -i butler.exe.bro >! butler.unbro.exe

amos at bowser in ~/tmp
$ ./butler.unbro.exe version
butler version v0.4.5

Updated test matrix:

program arch cross-compiled mingw-gcc version crashes?
butler 386 yes 4.6 no
butler x86_64 yes 4.6 yes
gbr x86_64 no 5.2 yes
bro x86_64 no 5.2 no
fasterthanlime commented 8 years ago

Alright branching off https://github.com/Bulat-Ziganshin/brotli/commit/e87bece2372d787b25ebed7df550699ee879eaca & merging with google/brotli at master, I've been able to reproduce the issue with bro.exe, so it doesn't seem to be a brotli-go issue after all.

amos at bowser in ~/tmp
$ brotli/tools/bro.exe -q 3 -i butler.exe > /dev/null; echo $?
0

amos at bowser in ~/tmp
$ brotli/tools/bro.exe -q 4 -i butler.exe > /dev/null; echo $?

This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information.
Assertion failed!

Program: C:\msys64\home\amos\tmp\brotli\tools\bro.exe
File: ./write_bits.h, Line 52

Expression: bits < 1UL << n_bits
3
kothar commented 8 years ago

Glad you were able to reproduce. I will start tagging the vendored upstream source id though, as you make a good point about tracking down matching versions.

fasterthanlime commented 8 years ago

The offending commit is https://github.com/google/brotli/commit/ea48ce5a6fa8a3224dd270a7f60428084e7b590d - reporting on upstream, hopefully they'll be responsive!

fasterthanlime commented 8 years ago

@kothar found a fix, if you're interested: https://github.com/google/brotli/issues/264#issuecomment-156140838

kothar commented 8 years ago

Good to know - that seems to be what was on that line before the last refactor.