Panic when running with multiple goroutines

[Till--H]

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.

[robbert229]

Very good finding. I will take a look at fixing this soon.

…

On Dec 10, 2017 5:03 AM, "Till Hohenberger" ***@***.***> 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 <#17>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADS2EPGo1-EjqFzZPHwABNuX2lGTO_spks5s-9aogaJpZM4Q8fBH> .

[henderjon]

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?

[robbert229]

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.

[henderjon]

Personally, I avoid mutexes when I don't need them. I'd say just document that goroutines shouldn't share an algorithm ...