search

sec51 / twofactor

Golang two factor authentication library
ISC License
217 stars 66 forks source link

Panic and Unit Tests fail on Go 1.11 #18

Closed MarkSonghurst closed 6 years ago

MarkSonghurst commented 6 years ago

I use the twofactor package in a daemon, which suddenly started to panic when I rebuilt and ran it using Go version 1.11 - this does not happen with Go 1.10

Running go test on the twofactor package produces the error. If you run go test --race then the error does not happen.

--- FAIL: TestVerificationFailures (0.00s)
panic: runtime error: slice bounds out of range [recovered]
    panic: runtime error: slice bounds out of range

goroutine 6 [running]:
testing.tRunner.func1(0xc0000b4200)
    /usr/local/go/src/testing/testing.go:792 +0x387
panic(0x550240, 0x682870)
    /usr/local/go/src/runtime/panic.go:513 +0x1b9
github.com/twofactor.TOTPFromBytes(0xc0000ca000, 0x93, 0xf3, 0x5798fd, 0x5, 0x0, 0xc0000b2100, 0x0)
    /home/msonghurst/go/src/github.com/twofactor/totp.go:560 +0xc9f
github.com/twofactor.TestVerificationFailures(0xc0000b4200)
    /home/msonghurst/go/src/github.com/twofactor/totp_test.go:171 +0x45c
testing.tRunner(0xc0000b4200, 0x5835a8)
    /usr/local/go/src/testing/testing.go:827 +0xbf
created by testing.(*T).Run
    /usr/local/go/src/testing/testing.go:878 +0x353
exit status 2
FAIL    github.com/twofactor    0.005s
[msonghurst@deimos twofactor]$ go version
go version go1.11 linux/amd64
MarkSonghurst commented 6 years ago

I found this comment in the Go 1.11 Release Notes:

The compiler now performs significantly more aggressive bounds-check and branch elimination. Notably, it now recognizes transitive relations, so if i<j and j<len(s), it can use these facts to eliminate the bounds check for s[i]. It also understands simple arithmetic such as s[i-10] and can recognize more inductive cases in loops. Furthermore, the compiler now uses bounds information to more aggressively optimize shift operations.

MarkSonghurst commented 6 years ago

The issue is in the sec51/convert dependency package. Here is a PR which resolves the issue. To work around what I suspect are Go 1.11 optimisations, I've used the Go binary package to do the endian conversions.

The convert package is stored static in the source control for the twofactor package, so will need to be updated here in twofactor as well.

es-lab commented 6 years ago

@MarkSonghurst I'm using go 1.10.3 and get the same error. I've copied the changes from your PR on top of the vendor ones and everything worked. @silenteh We need this merged ASAP. Thank you.

silenteh commented 6 years ago

@MarkSonghurst thanks for your help in fixing the issues with Golang 1.11 @es-lab the pull request was merged