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" notifications@github.com 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 .

[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 ...