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.

Sign up for GitHub

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

math/big: Rat.Denom needlessly mutates receiver #50473

Closed
rsc opened this issue Jan 6, 2022 · 1 comment
Closed

math/big: Rat.Denom needlessly mutates receiver #50473

rsc opened this issue Jan 6, 2022 · 1 comment
Labels
FrozenDueToAge
Milestone
Go1.18

Comments

@rsc
Copy link
Contributor

rsc commented Jan 6, 2022

big.Rat maintains the invariant that b.neg is false.
But then x.Denom needlessly contains

x.b.neg = false

at the top of the function.

If x.b.neg were in question, this would be a clear bug:
calling x.Denom must not change the value of x.

But since x.b.neg is always false, this is just a needless write,
which makes x.Denom trigger (arguably spurious) race reports
in programs that use x.Denom from multiple goroutines
or that use other code that calls it, such as big.Float's SetRat.

You'd certainly expect that given a globally initialized

var x *big.Rat = myValue()

it would be safe to do

f := new(big.Float).SetRat(x)

from multiple goroutines. Yet it is not - the race detector
notices the race writing useless false values to x.b.neg.

The fix is just to delete the assignment.
(Again, if x.b.neg could possibly be true,
then Denom would be changing the value of x!)

% cat x.go
package main

import "math/big"

func main() {
	z := big.NewRat(1, 2)
	c := make(chan int, 5)
	for i := 0; i < 5; i++ {
		go func() {
			z.Denom()
			c <- 1
		}()
	}
	for i := 0; i < 5; i++ {
		<-c
	}
}
% go run -race x.go
==================
WARNING: DATA RACE
Write at 0x00c000112060 by goroutine 8:
  math/big.(*Rat).Denom()
      /Users/rsc/go/src/math/big/rat.go:421 +0x44
  main.main.func1()
      /Users/rsc/x.go:10 +0x6a

Previous write at 0x00c000112060 by goroutine 7:
  math/big.(*Rat).Denom()
      /Users/rsc/go/src/math/big/rat.go:421 +0x44
  main.main.func1()
      /Users/rsc/x.go:10 +0x6a

Goroutine 8 (running) created at:
  main.main()
      /Users/rsc/x.go:9 +0x68

Goroutine 7 (finished) created at:
  main.main()
      /Users/rsc/x.go:9 +0x68
==================
Found 1 data race(s)
exit status 66
% cat xx.go
package main

import "math/big"

func main() {
	z := big.NewRat(1, 2)
	c := make(chan int, 5)
	for i := 0; i < 5; i++ {
		go func() {
			new(big.Float).SetRat(z)
			c <- 1
		}()
	}
	for i := 0; i < 5; i++ {
		<-c
	}
}
% go run -race xx.go
==================
WARNING: DATA RACE
Write at 0x00c000140020 by goroutine 8:
  math/big.(*Rat).Denom()
      /Users/rsc/go/src/math/big/rat.go:421 +0x164
  math/big.(*Float).SetRat()
      /Users/rsc/go/src/math/big/float.go:624 +0x18e
  main.main.func1()
      /Users/rsc/xx.go:10 +0x50

Previous write at 0x00c000140020 by goroutine 7:
  math/big.(*Rat).Denom()
      /Users/rsc/go/src/math/big/rat.go:421 +0x164
  math/big.(*Float).SetRat()
      /Users/rsc/go/src/math/big/float.go:624 +0x18e
  main.main.func1()
      /Users/rsc/xx.go:10 +0x50

Goroutine 8 (running) created at:
  main.main()
      /Users/rsc/xx.go:9 +0x68

Goroutine 7 (finished) created at:
  main.main()
      /Users/rsc/xx.go:9 +0x68
==================
Found 1 data race(s)
exit status 66
%
@rsc rsc added this to the Go1.18 milestone Jan 6, 2022
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/375935 mentions this issue: math/big: fix spurious race in Rat.Denom, Float.SetRat

jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
@rsc @jproberts
math/big: fix spurious race in Rat.Denom, Float.SetRat
609ca40 
Rat maintains the invariant that x.b.neg is always false,
but Rat.Denom was writing x.b.neg = false itself too.
That makes Rat.Denom a writing operation, when it should
be a read-only operation. That in turn makes it unsafe to
use from multiple goroutines, which is highly unexpected.
Make it read-only and therefore race-free again.

Fixes golang#50473.

Change-Id: I97b87913954511e5200c0665d16b9ed63422e505
Reviewed-on: https://go-review.googlesource.com/c/go/+/375935
Trust: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@golang golang locked and limited conversation to collaborators Jan 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
2 participants
@rsc @gopherbot