math/big: Rat.Float64 not safe for concurrent use #34919
Comments
|
As another data point, if z is a Float and x is a Rat, z.SetRat(x) calls x.Denom, meaning that z.SetRat(x) is unsafe to call concurrently with other uses of x, even though it seems to only be reading x. |
|
WRT to #33792, |
|
@ericlagergren Indeed, but the primary reason for returning a reference was speed, if I remember correctly. In retrospect, and that's entirely my fault, the implementation probably should have returned a fixed (new) one. I'm looking into these. |
griesemer
added
the
NeedsFix
|
Change https://golang.org/cl/201205 mentions this issue: |
|
I'm trying to determine if this qualifies for backporting. It looks like a serious issue, however I understand it (along with #33792) has existed for many years. The only workaround I can think of right now is to rewrite code to use Rat.Float64 and Rat.Denom differently for Go 1.13 and older. Are there other workarounds available for Go 1.13 and 1.12? Based on analysis in #36605 (comment), this issue is causing data races in |
|
Change https://golang.org/cl/233321 mentions this issue: |
…ent use Do not modify the underlying Rat denominator when calling one of the accessors Float32, Float64; verify that we don't modify the Rat denominator when calling Inv, Sign, IsInt, Num. For #36689. For #34919. For #33792. Change-Id: Ife6d1252373f493a597398ee51e7b5695b708df5 Reviewed-on: https://go-review.googlesource.com/c/go/+/201205 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-on: https://go-review.googlesource.com/c/go/+/233321 Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
In math/big, Rat.Float64 modifies x.b.abs by calling set(natNone), which makes it unsafe for concurrent use.
But you would really expect that a reading method like Float64 would be safe.
In fact go/constant's Float64Val assumes this, unless you also want to say that constant.Float64Val is not safe for concurrent use either.
But then you'd have to track all the calls to that, like go/types.roundFloat64, called from the types.Checker, and so on.
It seems like the solution has to be to make Rat.Float64 safe for concurrent use.
It would be fine to just not modify x.b.abs there and do 'b = natOne', no?
This is the root cause of the original go/packages race reported in #31749.
It may also be a mistake to have fixed #33792 by declaring Denom to be unsafe for concurrent use,
for many of the same reasons.
That could have returned a fixed 1 int as well.
The text was updated successfully, but these errors were encountered: