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

Jump to bottom

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
Closed

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

gopherbot opened this issue Jan 22, 2020 · 14 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.13.12

Comments

@gopherbot
Copy link

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

@gopherbot backport please 1.12 1.13
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Jan 22, 2020
@gopherbot gopherbot mentioned this issue Jan 22, 2020
Closed
@gopherbot gopherbot added this to the Go1.13.7 milestone Jan 22, 2020
@toothrot
Copy link
Contributor

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

@matloob
Copy link
Contributor

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.

@dmitshur dmitshur modified the milestones: Go1.13.7, Go1.13.8 Jan 27, 2020
@toothrot toothrot modified the milestones: Go1.13.8, Go1.13.9 Feb 12, 2020
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 2, 2020
@dmitshur
Copy link
Contributor

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

@dmitshur
Copy link
Contributor

dmitshur commented Mar 13, 2020 •
edited

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?

@cagedmantis cagedmantis modified the milestones: Go1.13.9, Go1.13.10 Mar 19, 2020
@andybons
Copy link
Member

ping @griesemer

@andybons
Copy link
Member

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?

@griesemer
Copy link
Contributor

griesemer commented Apr 7, 2020 •
edited

[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.)

@andybons andybons modified the milestones: Go1.13.10, Go1.13.11 Apr 8, 2020
@dmitshur
Copy link
Contributor

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.

@dmitshur dmitshur removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 11, 2020
@gopherbot
Copy link
Author

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

@gopherbot
Copy link
Author

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

@gopherbot
Copy link
Author

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

@andybons andybons modified the milestones: Go1.13.11, Go1.13.12 May 14, 2020
@cagedmantis cagedmantis added the CherryPickApproved Used during the release process for point releases label May 21, 2020
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label May 21, 2020
@gopherbot
Copy link
Author

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

@gopherbot
Copy link
Author

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

gopherbot pushed a commit that referenced this issue May 27, 2020
@griesemer @dmitshur
[release-branch.go1.13] math/big: make Rat accessors safe for concurr…
444ad95 
…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
@griesemer @dmitshur
[release-branch.go1.13] math/big: normalize unitialized denominators …
7f1ef49 
…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

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

gopherbot pushed a commit that referenced this issue May 27, 2020
@griesemer @dmitshur
[release-branch.go1.13] math/big: make Rat.Denom side-effect free
90fe4a7 
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 mentioned this issue May 28, 2020
Closed
@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.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.13.12
Development

No branches or pull requests
7 participants
@toothrot @cagedmantis @andybons @dmitshur @griesemer @gopherbot @matloob