robbert229
Panic when running with multiple goroutines
Hi,
I've discovered that this library panics when run with multiple goroutines.
Considering the following minimal example:
package main
import (
"sync"
"github.com/robbert229/jwt"
)
func main() {
algorithm := jwt.HmacSha256("ThisIsTheSecret")
var wg sync.WaitGroup
noRoutines := 10
wg.Add(noRoutines)
for i := 0; i < noRoutines; i++ {
go decode(&algorithm, &wg)
}
wg.Wait()
}
func decode(algorithm *jwt.Algorithm, wg *sync.WaitGroup) {
defer wg.Done()
claims := jwt.NewClaim()
claims.Set("Role", "Admin")
token, err := algorithm.Encode(claims)
if err != nil {
panic(err)
}
for index := 0; index < 100; index++ {
_, err = algorithm.Decode(token)
if err != nil {
panic(err)
}
}
}
Almost every run of this program leads to a panic like
panic: d.nx != 0
goroutine 7 [running]:
crypto/sha256.(*digest).checkSum(0xc42003fd10, 0x0, 0x0, 0x0, 0x0)
/usr/local/Cellar/go/1.9.2/libexec/src/crypto/sha256/sha256.go:157 +0x29e
crypto/sha256.(*digest).Sum(0xc420084080, 0x0, 0x0, 0x0, 0x60, 0x5d, 0x0)
/usr/local/Cellar/go/1.9.2/libexec/src/crypto/sha256/sha256.go:131 +0x69
crypto/hmac.(*hmac).Sum(0xc420052060, 0x0, 0x0, 0x0, 0x5d, 0x0, 0x0)
/usr/local/Cellar/go/1.9.2/libexec/src/crypto/hmac/hmac.go:46 +0x56
gitlab.com/jwt-test/vendor/github.com/robbert229/jwt.(*Algorithm).sum(0xc42000a060, 0x0, 0x0, 0x0, 0x5d, 0x0, 0x0)
/myGopath/jwt-test/vendor/github.com/robbert229/jwt/algorithms.go:32 +0x51
gitlab.com/jwt-test/vendor/github.com/robbert229/jwt.(*Algorithm).Sign(0xc42000a060, 0xc4200e4000, 0x5d, 0x1104718, 0x1, 0xc4200d4090, 0x2c)
/myGopath/jwt-test/vendor/github.com/robbert229/jwt/algorithms.go:50 +0xff
gitlab.com/jwt-test/vendor/github.com/robbert229/jwt.(*Algorithm).Encode(0xc42000a060, 0xc420090010, 0x110490b, 0x4, 0xc42009e1b8, 0x0)
/myGopath/jwt-test/vendor/github.com/robbert229/jwt/algorithms.go:76 +0x1e7
main.decode(0xc42000a060, 0xc4200160d0)
/myGopath/jwt-test/main.go:24 +0xca
created by main.main
/myGopath/jwt-test/main.go:15 +0xf3
exit status 2
We should at least document how the expected concurrenct usage scenario for this library is.
Very good finding. I will take a look at fixing this soon.
On Dec 10, 2017 5:03 AM, "Till Hohenberger" [email protected] wrote:
Hi,
I've discovered that this library panics when run with multiple goroutines.
Considering the following minimal example:
package main
import ( "sync"
"github.com/robbert229/jwt" )
func main() { algorithm := jwt.HmacSha256("ThisIsTheSecret") var wg sync.WaitGroup noRoutines := 10 wg.Add(noRoutines) for i := 0; i < noRoutines; i++ { go decode(&algorithm, &wg) } wg.Wait() }
func decode(algorithm *jwt.Algorithm, wg *sync.WaitGroup) { defer wg.Done() claims := jwt.NewClaim() claims.Set("Role", "Admin") token, err := algorithm.Encode(claims) if err != nil { panic(err) } for index := 0; index < 100; index++ { _, err = algorithm.Decode(token) if err != nil { panic(err) } } }
Almost every run of this program leads to a panic like
panic: d.nx != 0
goroutine 7 [running]: crypto/sha256.(*digest).checkSum(0xc42003fd10, 0x0, 0x0, 0x0, 0x0) /usr/local/Cellar/go/1.9.2/libexec/src/crypto/sha256/sha256.go:157 +0x29e crypto/sha256.(*digest).Sum(0xc420084080, 0x0, 0x0, 0x0, 0x60, 0x5d, 0x0) /usr/local/Cellar/go/1.9.2/libexec/src/crypto/sha256/sha256.go:131 +0x69 crypto/hmac.(*hmac).Sum(0xc420052060, 0x0, 0x0, 0x0, 0x5d, 0x0, 0x0) /usr/local/Cellar/go/1.9.2/libexec/src/crypto/hmac/hmac.go:46 +0x56gitlab.com/jwt-test/vendor/github.com/robbert229/jwt.(*Algorithm).sum(0xc42000a060, 0x0, 0x0, 0x0, 0x5d, 0x0, 0x0) /myGopath/jwt-test/vendor/github.com/robbert229/jwt/algorithms.go:32 +0x51gitlab.com/jwt-test/vendor/github.com/robbert229/jwt.(*Algorithm).Sign(0xc42000a060, 0xc4200e4000, 0x5d, 0x1104718, 0x1, 0xc4200d4090, 0x2c) /myGopath/jwt-test/vendor/github.com/robbert229/jwt/algorithms.go:50 +0xffgitlab.com/jwt-test/vendor/github.com/robbert229/jwt.(*Algorithm).Encode(0xc42000a060, 0xc420090010, 0x110490b, 0x4, 0xc42009e1b8, 0x0) /myGopath/jwt-test/vendor/github.com/robbert229/jwt/algorithms.go:76 +0x1e7 main.decode(0xc42000a060, 0xc4200160d0) /myGopath/jwt-test/main.go:24 +0xca created by main.main /myGopath/jwt-test/main.go:15 +0xf3 exit status 2
We should at least document how the expected concurrenct usage scenario for this library is.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/robbert229/jwt/issues/17, or mute the thread https://github.com/notifications/unsubscribe-auth/ADS2EPGo1-EjqFzZPHwABNuX2lGTO_spks5s-9aogaJpZM4Q8fBH .
Digging into this, it would seem that the issue is that the lib is writing, summing, resetting the underlying hash.Hash. Doing this across goroutines, I think, causes a race condition. At some point, the hash.Hash is being written to twice (or more) before it's being reset. You could add a mutex or a channel to try to avoid it or stop passing the algorithm around.
package main
import (
"log"
"sync"
"github.com/capdig/jwt"
)
func main() {
var wg sync.WaitGroup
noRoutines := 100
wg.Add(noRoutines)
for i := 0; i < noRoutines; i++ {
go decode(jwt.HmacSha256("ThisIsTheSecret"), &wg)
}
wg.Wait()
}
func decode(algorithm jwt.Algorithm, wg *sync.WaitGroup) {
defer wg.Done()
log.Println("done")
claims := jwt.NewClaim()
claims.Set("Role", "Admin")
token, err := algorithm.Encode(claims)
if err != nil {
panic(err)
}
for index := 0; index < 10000; index++ {
_, err = algorithm.Decode(token)
if err != nil {
panic(err)
}
}
}
Granted this would incur a bit more overhead as, in this example, we're creating a lot more hash.Hashes. But as far as I can tell, doing so avoids the panic. I don't think this is too much to ask as JWTs by definition are small and their signing isn't done more than once or twice per request.
Thoughts?
This makes sense. Adding a mutex for synchronization would work, and I think that's what I will do (unless you want to submit a PR 😄 ).
In my projects where I use this I end up giving each goroutine a copy of the data. Though It shouldn't break for others who don't do this.
Personally, I avoid mutexes when I don't need them. I'd say just document that goroutines shouldn't share an algorithm ...