coinbase / kryptology

Apache License 2.0
847 stars 123 forks source link

Runtime error when calling `g2.MulScalar` concurrently #26

Closed boumba100 closed 2 years ago

boumba100 commented 2 years ago

I’m getting the following error when I call g2.MulScalar concurrently:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x4ed760]

goroutine 19 [running]:
github.com/coinbase/kryptology/pkg/core/curves/native/bls12-381.(*G2).toBytesJacobi(0xc0001500a0, 0x0)
    go/pkg/mod/github.com/coinbase/kryptology@v1.5.4/pkg/core/curves/native/bls12-381/g2.go:274 +0x40
github.com/coinbase/kryptology/pkg/core/curves/native/bls12-381.(*G2).MulScalar(0xc00008c360, 0x5312e0, 0x537740, 0x753700)
    go/pkg/mod/github.com/coinbase/kryptology@v1.5.4/pkg/core/curves/native/bls12-381/g2.go:502 +0x148
github.com/coinbase/kryptology/pkg/core/curves.(*PointBls12381G2).Mul(0xc000010038, {0x6968f0, 0xc00000e060})
    go/pkg/mod/github.com/coinbase/kryptology@v1.5.4/pkg/core/curves/bls12381_curve.go:704 +0x7b
github.com/coinbase/kryptology/pkg/signatures/bbs.(*SecretKey).PublicKey(0xc000012020)
    go/pkg/mod/github.com/coinbase/kryptology@v1.5.4/pkg/signatures/bbs/secret_key.go:132 +0x7d
github.com/coinbase/kryptology/pkg/signatures/bbs.NewKeys(0x6961b0)
    go/pkg/mod/github.com/coinbase/kryptology@v1.5.4/pkg/signatures/bbs/secret_key.go:78 +0x3c

It looks like the ‘q’ variable somehow gets dereferenced.

Since Golang is quite new to me, I don’t have a solution at the moment but I think pointing this out could be useful.

Here is some code to reproduce the error

    package main

    import (
        "time"

        "github.com/coinbase/kryptology/pkg/core/curves"
        "github.com/coinbase/kryptology/pkg/signatures/bbs"
    )

    func main() {
        go newKeys()
        go newKeys()

        time.Sleep(time.Second)
    }

    func newKeys() {
        curve := curves.BLS12381(&curves.PointBls12381G2{})

        bbs.NewKeys(curve)
    }
kilic commented 2 years ago

@boumba100 It seems like this library had some benefits from https://github.com/kilic/bls12-381 for bls12-381.

In there we already stated that group or pairing engine instances are not suitable for concurrency since we preallocate temporary variables to gain some efficiency. So for each thread a new instance should be created.

mikelodder7 commented 2 years ago

@kilic this is a fork from your library that we audited. We did submit some PRs back to you but they were ignored. I'm open to doing so again if you are.

dB2510 commented 2 years ago

@mikelodder7 Any updates on this issue? What's the timeline that this issue will get resolved?

mikelodder7 commented 2 years ago

Probably in the next few days. We’re just about done

mikelodder7 commented 2 years ago

Should be fixed with #49