-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
avoid race condition in MakeAffine/ValidatePairing #59
Conversation
19712af
to
70a02ce
Compare
pairing/bn254/suite.go
Outdated
// NB: Not safe for concurrent calls | ||
func (s *Suite) ValidatePairing(p1, p2, inv1, inv2 kyber.Point) bool { | ||
p2.(*pointG2).g.MakeAffine() | ||
inv2.(*pointG2).g.MakeAffine() | ||
p2.Clone().(*pointG2).g.MakeAffine() | ||
inv2.Clone().(*pointG2).g.MakeAffine() | ||
return s.Pair(p1, p2).Equal(s.Pair(inv1, inv2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think Clone() works like that, it returns a clone of the point, no?
You should be creating local variables if you want to use Clone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦♂️ indeed you are right. I got mixed up with MakeAffine
modifying the point itself...
g.x.Set(t) | ||
g.z.SetOne() | ||
g.t.SetOne() | ||
c.Set(g) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of this Set here, I don't think using a Cloned g
actually changes much, except it makes the timing of a race more difficult to "hit" in prod/tests...
This function should definitively have a "not safe for concurrent use" in the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this bit is dicey. I added the "not safe for concurrent use" warning above the function.
6d1a47b
to
8eee318
Compare
@@ -132,10 +132,13 @@ func (s *Suite) Pair(p1 kyber.Point, p2 kyber.Point) kyber.Point { | |||
return s.GT().Point().(*pointGT).Pair(p1, p2) | |||
} | |||
|
|||
// NB: Not safe for concurrent calls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure this is actually true since now you clone the inputs 🤔
Addressing issues found in drand/drand#1304 (comment)