Skip to content
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

go/types, math/big: data race in go/types due to math/big.Rat accessors unsafe for concurrent use [1.13 backport] #36689

Closed
gopherbot opened this issue Jan 22, 2020 · 14 comments

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 22, 2020

@matloob requested issue #36687 to be considered for backport to the next 1.13 minor release.

@gopherbot backport please 1.12 1.13

@toothrot
Copy link
Contributor

@toothrot toothrot commented Jan 22, 2020

@matloob Could you please include a short rationale for the backport, in accordance with https://golang.org/wiki/MinorReleases?

Loading

@matloob
Copy link
Contributor

@matloob matloob commented Jan 22, 2020

Including the rationale from the original change. go/types and go/packages (through the go/types dependency) will have a race in 1.13 without this fix.

The race in go/packages reported in #31749 was fixed at tip, but is still an issue for Go 1.13 and earlier, and is showing up in the go/packages race failure reported in #36605.

That race is caused by a race in go/types which in turn is caused by go/constant.Float64Val() calling Rat.Float64 in math/big, which has a race reported in #34919 , and subsequently fixed in tip.

Fixing this race in go/types on Go 1.13 and earlier requires backporting CL 201205 to those releases. (Alternatively, we could fix the race by adding a workaround in go/constant or go/types, but that would be more work). I'm not sure if fixing this is worth a backport but we might as well have the discussion. If we decide not to go with the backport, we'll have to accept a race in go/packages and disable some of its tests in race mode in Go 1.13 and earlier.

Loading

@dmitshur dmitshur removed this from the Go1.13.7 milestone Jan 27, 2020
@dmitshur dmitshur added this to the Go1.13.8 milestone Jan 27, 2020
@toothrot toothrot removed this from the Go1.13.8 milestone Feb 12, 2020
@toothrot toothrot added this to the Go1.13.9 milestone Feb 12, 2020
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Mar 10, 2020

There is a new comment at #36605 (comment) that is relevant to this backport issue. /cc @matloob

Loading

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Mar 13, 2020

We've discussed this cherry pick candidate in a release meeting.

To recap the latest status of this candidate, we know this is a serious problem and we have been investigating to see if there is a workaround. Based on various discussions, it doesn't seem there is a good workaround available, and we should only consider backporting math/big CL(s) to Go 1.13 that are necessary to fix #36687.

I understand the primary math/big CL that needs to be backported is:

  • CL 201205 - math/big: make Rat accessors safe for concurrent use

@matloob has said earlier in #36605 (comment):

I tried cherrypicking the fix CL (golang.org/cl/201205) and running the tests in race mode and it seems to fix the issue.

So CL 201205 should be sufficient to resolve issue #36687.

However, the commit message of that CL says:

Fixes #34919.
Reopens #33792.

Issue #33792 took two more CLs to resolve:

To make a decision on whether to make this backport, we need to determine whether it is safe to backport just CL 201205, or if that requires CLs 202997 and 203059 to be backported as well and if all 3 CLs are safe to backport.

@griesemer Do you know if backporting CL 201205 to 1.13 by itself is safe, or is it unsafe to do that without also backporting CL 202997 and CL 203059?

Loading

@cagedmantis cagedmantis removed this from the Go1.13.9 milestone Mar 19, 2020
@cagedmantis cagedmantis added this to the Go1.13.10 milestone Mar 19, 2020
@andybons
Copy link
Member

@andybons andybons commented Mar 26, 2020

ping @griesemer

Loading

@andybons
Copy link
Member

@andybons andybons commented Apr 7, 2020

This is blocked and can’t be approved yet until we have an answer to:

@griesemer Do you know if backporting CL 201205 to 1.13 by itself is safe, or is it unsafe to do that without also backporting CL 202997 and CL 203059?

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Apr 7, 2020

[edited]

@andybons I would port all three. They all interact in subtle ways with the same code and it seems dangerous to just apply one or two. We know that together they do what they are intended to do.

(At the very least, it would require a careful analysis of what's going on if we only apply one of them, and we may even run into merge conflicts, which would defeat the purpose of a back-port.)

Loading

@andybons andybons removed this from the Go1.13.10 milestone Apr 8, 2020
@andybons andybons added this to the Go1.13.11 milestone Apr 8, 2020
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented May 11, 2020

I've created a local branch and cherry-picked the 3 math/big CLs in order to see if any problems could be discovered from that process.

The 3 CLs applied cleanly to the latest release-branch.go1.13 branch. all.bash and go test std cmd reported no failures. I ran the reproducer program mentioned in #36687 (comment), it reported data races using the current release-branch.go1.13, but not after the 3 CLs were applied.

This seems like a critical problem for users that are affected, and no workaround exists (other than "upgrade to Go 1.14", which is not a reasonable workaround). The patch is well-proven (via Go 1.14), so I am in favor of approving this backport.

Loading

@gopherbot
Copy link
Author

@gopherbot gopherbot commented May 11, 2020

Change https://golang.org/cl/233323 mentions this issue: [release-branch.go1.13] math/big: make Rat.Denom side-effect free

Loading

@gopherbot
Copy link
Author

@gopherbot gopherbot commented May 11, 2020

Change https://golang.org/cl/233321 mentions this issue: [release-branch.go1.13] math/big: make Rat accessors safe for concurrent use

Loading

@gopherbot
Copy link
Author

@gopherbot gopherbot commented May 11, 2020

Change https://golang.org/cl/233322 mentions this issue: [release-branch.go1.13] math/big: normalize unitialized denominators ASAP

Loading

@andybons andybons removed this from the Go1.13.11 milestone May 14, 2020
@andybons andybons added this to the Go1.13.12 milestone May 14, 2020
@gopherbot
Copy link
Author

@gopherbot gopherbot commented May 27, 2020

Closed by merging 7f1ef49 to release-branch.go1.13.

Loading

@gopherbot gopherbot closed this May 27, 2020
@gopherbot
Copy link
Author

@gopherbot gopherbot commented May 27, 2020

Closed by merging 444ad95 to release-branch.go1.13.

Loading

gopherbot pushed a commit that referenced this issue May 27, 2020
…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>
gopherbot pushed a commit that referenced this issue May 27, 2020
…ASAP

A Rat is represented via a quotient a/b where a and b are Int values.
To make it possible to use an uninitialized Rat value (with a and b
uninitialized and thus == 0), the implementation treats a 0 denominator
as 1.

For each operation we check if the denominator is 0, and then treat
it as 1 (if necessary). Operations that create a new Rat result,
normalize that value such that a result denominator 1 is represened
as 0 again.

This CL changes this behavior slightly: 0 denominators are still
interpreted as 1, but whenever we (safely) can, we set an uninitialized
0 denominator to 1. This simplifies the code overall.

Also: Improved some doc strings.

Preparation for addressing issue #33792.

For #36689.
For #33792.

Change-Id: I3040587c8d0dad2e840022f96ca027d8470878a0
Reviewed-on: https://go-review.googlesource.com/c/go/+/202997
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/233322
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
@gopherbot
Copy link
Author

@gopherbot gopherbot commented May 27, 2020

Closed by merging 90fe4a7 to release-branch.go1.13.

Loading

gopherbot pushed a commit that referenced this issue May 27, 2020
A Rat is represented via a quotient a/b where a and b are Int values.
To make it possible to use an uninitialized Rat value (with a and b
uninitialized and thus == 0), the implementation treats a 0 denominator
as 1.

Rat.Num and Rat.Denom return pointers to these values a and b. Because
b may be 0, Rat.Denom used to first initialize it to 1 and thus produce
an undesirable side-effect (by changing the Rat's denominator).

This CL changes Denom to return a new (not shared) *Int with value 1
in the rare case where the Rat was not initialized. This eliminates
the side effect and returns the correct denominator value.

While this is changing behavior of the API, the impact should now be
minor because together with (prior) CL https://golang.org/cl/202997,
which initializes Rats ASAP, Denom is unlikely used to access the
denominator of an uninitialized (and thus 0) Rat. Any operation that
will somehow set a Rat value will ensure that the denominator is not 0.

Fixes #36689.
For #33792.
For #3521.

Change-Id: I0bf15ac60513cf52162bfb62440817ba36f0c3fc
Reviewed-on: https://go-review.googlesource.com/c/go/+/203059
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/233323
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
@dmitshur dmitshur changed the title go/types: data race in representableConst in Go 1.13 and earlier [1.13 backport] go/types, math/big: data race in go/types due to math/big.Rat accessors unsafe for concurrent use [1.13 backport] Jun 1, 2020
@golang golang locked and limited conversation to collaborators Jun 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants