-
Notifications
You must be signed in to change notification settings - Fork 360
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
bug: Race condition on Hint #998
Comments
BTW, the fix from client's code:
|
but I do think it's an easy fix from the library's code, just de-dup the duplicated memory address |
As far as I recall, we use a pool to reuse the inputs and outputs from the hint. So it would be correct to instead assign to output value using Could you try that in the hint you would do: func idivFunc(mod *big.Int, inputs []*big.Int, outputs []*big.Int) error {
zero := big.NewInt(0)
if inputs[0].Cmp(zero) == 0 && inputs[1].Cmp(zero) == 0 {
outputs[0].Set(big.NewInt(1))
outputs[1].Set(big.NewInt(0))
return nil
} else if inputs[1].Cmp(zero) == 0 {
outputs[0].Set(big.NewInt(0))
outputs[1].Set(inputs[0])
return nil
}
q, r = new(big.Int).DivMod(inputs[0], inputs[1], outputs[1])
outputs[0].Set(q)
outputs[1].Set(r)
return nil
}
func Permute(_ *big.Int, inputs []*big.Int, outputs []*big.Int) error {
if len(inputs) != len(outputs) {
panic("Input and Output length should be the same")
}
var newOdd []*big.Int = make([]*big.Int, 0)
var newEnd []*big.Int = make([]*big.Int, 0)
for i := 0; i < len(inputs); i++ {
if inputs[i].Cmp(big.NewInt(1<<21)) == 1 {
newEnd = append(newEnd, inputs[i])
} else {
newOdd = append(newOdd, inputs[i])
}
}
for i := 0; i < len(newOdd); i++ {
outputs[i].Set(newOdd[i])
}
for i := 0; i < len(newEnd); i++ {
outputs[len(newOdd)+i].Set(newEnd[i])
}
return nil
} |
The doc should also clarify following code is not allowed: func Permute(_ *big.Int, inputs []*big.Int, outputs []*big.Int) error {
if len(inputs) != len(outputs) {
panic("Input and Output length should be the same")
}
var newOdd []*big.Int = make([]*big.Int, 0)
var newEnd []*big.Int = make([]*big.Int, 0)
for i := 0; i < len(inputs); i++ {
if inputs[i].Cmp(big.NewInt(1<<21)) == 1 {
newEnd = append(newEnd, inputs[i])
} else {
newOdd = append(newOdd, inputs[i])
}
}
for i := 0; i < len(newOdd); i++ {
outputs[i] = newOdd[i]
}
for i := 0; i < len(newEnd); i++ {
outputs[len(newOdd)+i] = newEnd[i]
}
return nil
} This can cause race condition. It should emphiaze that user can ONLY "Set" the output value, and users cannot assign any other new/old pointer to the output. |
We found a race condition on SolveWithHint function, we provide a minimal example to reproduce the bug.
Description
Expected Behavior
Pass the test
Actual Behavior
sometimes it's div by zero, sometimes it's unsatisfied constraint.
Possible Fix
Either fix the race condition, or just specify the following in your documentation:
In a hint function, you cannot directly copy the input to the output (or even in a permuted way).
Steps to Reproduce
Your Environment
Cause of the problem:
In this file:
https://github.com/Consensys/gnark/blob/5f1643d98071aabc5abbe89573e4c153cd1c6b0e/constraint/bn254/solver.go#L240C10-L240C10
this part:
There is a race condition if output is a copy of input. For example:
Then in this case, thread 2 will be in trouble because q is recycled by Thread 1 and may be later used by other processes causing a race condition.
The text was updated successfully, but these errors were encountered: