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

DATA RACE: rand.NewSource is not safe for concurrent usage. #10988

Closed
aanm opened this issue Apr 15, 2020 · 1 comment · Fixed by #10997 or #32542
Closed

DATA RACE: rand.NewSource is not safe for concurrent usage. #10988

aanm opened this issue Apr 15, 2020 · 1 comment · Fixed by #10997 or #32542
Assignees
Labels
kind/bug This is a bug in the Cilium logic.

Comments

@aanm
Copy link
Member

aanm commented Apr 15, 2020

we have a couple usages of this function:

source 1000f70

daemon/cmd/status.go:53:var randGen = rand.New(rand.NewSource(time.Now().UnixNano()))
pkg/kvstore/etcd.go:68: randGen = rand.New(rand.NewSource(time.Now().UnixNano()))
pkg/lock/stoppable_waitgroup_test.go:170:       randGen := rand.New(rand.NewSource(time.Now().UnixNano()))
pkg/proxy/proxy.go:147: portRandomizer = rand.New(rand.NewSource(time.Now().UnixNano()))
pkg/rand/rand_name.go:26:       randGen = rand.New(rand.NewSource(time.Now().UnixNano()))
test/helpers/utils.go:43:var randGen = rand.New(rand.NewSource(time.Now().UnixNano()))
test/runtime/monitor.go:31:var randGen = rand.New(rand.NewSource(time.Now().UnixNano()))

The documentation states it's not safe for concurrent usage:

// NewSource returns a new pseudo-random Source seeded with the given value.
// Unlike the default Source used by top-level functions, this source is not
// safe for concurrent use by multiple goroutines.

Can cause issues similar to:

2020-04-15T09:20:36.572247228Z WARNING: DATA RACE
2020-04-15T09:20:36.572327995Z Read at 0x00c0002de000 by goroutine 391:
2020-04-15T09:20:36.572368052Z   math/rand.(*rngSource).Uint64()
2020-04-15T09:20:36.572404065Z       /usr/local/go/src/math/rand/rng.go:239 +0x3e
2020-04-15T09:20:36.572451459Z   math/rand.(*Rand).Uint64()
2020-04-15T09:20:36.572456839Z       /usr/local/go/src/math/rand/rand.go:93 +0x78
2020-04-15T09:20:36.572492861Z   github.com/cilium/cilium/pkg/kvstore.(*etcdClient).waitForInitLock.func1()
2020-04-15T09:20:36.57249782Z       /go/src/github.com/cilium/cilium/pkg/kvstore/etcd.go:363 +0xa0
2020-04-15T09:20:36.572531331Z 
2020-04-15T09:20:36.572594608Z Previous write at 0x00c0002de000 by goroutine 290:
2020-04-15T09:20:36.572630276Z   math/rand.(*rngSource).Uint64()
2020-04-15T09:20:36.572634947Z       /usr/local/go/src/math/rand/rng.go:239 +0x54
2020-04-15T09:20:36.572668487Z   math/rand.(*Rand).Uint64()
2020-04-15T09:20:36.572672987Z       /usr/local/go/src/math/rand/rand.go:93 +0x78
2020-04-15T09:20:36.572706705Z   github.com/cilium/cilium/pkg/kvstore.(*etcdClient).waitForInitLock.func1()
2020-04-15T09:20:36.572711435Z       /go/src/github.com/cilium/cilium/pkg/kvstore/etcd.go:363 +0xa0
2020-04-15T09:20:36.5727441Z 
2020-04-15T09:20:36.572778832Z Goroutine 391 (running) created at:
2020-04-15T09:20:36.572814227Z   github.com/cilium/cilium/pkg/kvstore.(*etcdClient).waitForInitLock()
2020-04-15T09:20:36.572818802Z       /go/src/github.com/cilium/cilium/pkg/kvstore/etcd.go:351 +0x8c
2020-04-15T09:20:36.572852416Z   github.com/cilium/cilium/pkg/kvstore.(*etcdClient).isConnectedAndHasQuorum()
2020-04-15T09:20:36.572857049Z       /go/src/github.com/cilium/cilium/pkg/kvstore/etcd.go:396 +0x28d
2020-04-15T09:20:36.572909971Z   github.com/cilium/cilium/pkg/kvstore.(*etcdClient).Connected.func1()
2020-04-15T09:20:36.572914532Z       /go/src/github.com/cilium/cilium/pkg/kvstore/etcd.go:411 +0x5b
2020-04-15T09:20:36.572957404Z 
2020-04-15T09:20:36.572992805Z Goroutine 290 (running) created at:
2020-04-15T09:20:36.57302938Z   github.com/cilium/cilium/pkg/kvstore.(*etcdClient).waitForInitLock()
2020-04-15T09:20:36.573034089Z       /go/src/github.com/cilium/cilium/pkg/kvstore/etcd.go:351 +0x8c
2020-04-15T09:20:36.57307812Z   github.com/cilium/cilium/pkg/kvstore.(*etcdClient).isConnectedAndHasQuorum()
2020-04-15T09:20:36.573093372Z       /go/src/github.com/cilium/cilium/pkg/kvstore/etcd.go:396 +0x28d
2020-04-15T09:20:36.573129588Z   github.com/cilium/cilium/pkg/kvstore.(*etcdClient).Connected.func1()
2020-04-15T09:20:36.573134528Z       /go/src/github.com/cilium/cilium/pkg/kvstore/etcd.go:411 +0x5b
@aanm aanm added kind/bug This is a bug in the Cilium logic. priority/release-blocker labels Apr 15, 2020
@tklauser
Copy link
Member

Most of these were introduced by #10575 in an attempt to avoid unrelated packages interfering with each others PRNG state. I didn't read the docs carefully enough so didn't spot that this method is not concurrency-safe 😕

It seems there currently isn't an easy way to get a non-global, concurrency-safe PRNG source with the current math/rand implementation, see golang/go#21393

My proposal to solved this would be use something similar to lockedSource from math/rand in our pkg/rand and use that for the time being:

https://github.com/golang/go/blob/8f53fad035ccc580859f7b063ae8be30b009a6be/src/math/rand/rand.go#L382-L385

tklauser added a commit that referenced this issue Apr 16, 2020
The godoc comment for package `math/rand` states:

  The default Source is safe for concurrent use by multiple goroutines, but
  Sources created by NewSource are not.

The global source in `math/rand` would be concurrency-safe, but its
state may be interfered with from unrelated packages, e.g.

```
package a

func init() {
	math.Seed(time.Now().UnixNano()
}
```

and

```
package b

func init() {
	math.Seed(0)
}
```

Depending on the package load order, the PRNG state would be predictable
for all users (i.e.  always seeded using the fixed value 0), including
`package a`.

To fix this, provide a concurrency-safe wrapper around `math/rand.Rand`
in `pkg/rand` and use it to replace all existing users of
`math/rand.New(rand.NewSource(...))`

Fixes #10988

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
borkmann pushed a commit that referenced this issue Apr 16, 2020
The godoc comment for package `math/rand` states:

  The default Source is safe for concurrent use by multiple goroutines, but
  Sources created by NewSource are not.

The global source in `math/rand` would be concurrency-safe, but its
state may be interfered with from unrelated packages, e.g.

```
package a

func init() {
	math.Seed(time.Now().UnixNano()
}
```

and

```
package b

func init() {
	math.Seed(0)
}
```

Depending on the package load order, the PRNG state would be predictable
for all users (i.e.  always seeded using the fixed value 0), including
`package a`.

To fix this, provide a concurrency-safe wrapper around `math/rand.Rand`
in `pkg/rand` and use it to replace all existing users of
`math/rand.New(rand.NewSource(...))`

Fixes #10988

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
tgraf added a commit that referenced this issue Jul 15, 2020
[ upstream commit fac5dde ]

Backport notes:
 Backported as requirement for b95650b ("etcd: Shuffle list of etcd endpoints")

The godoc comment for package `math/rand` states:

The default Source is safe for concurrent use by multiple goroutines, but
Sources created by NewSource are not.

The global source in `math/rand` would be concurrency-safe, but its
state may be interfered with from unrelated packages, e.g.

```
package a

func init() {
    math.Seed(time.Now().UnixNano()
}
```

and

```
package b

func init() {
    math.Seed(0)
}
```

Depending on the package load order, the PRNG state would be predictable
for all users (i.e.  always seeded using the fixed value 0), including
`package a`.

To fix this, provide a concurrency-safe wrapper around `math/rand.Rand`
in `pkg/rand` and use it to replace all existing users of
`math/rand.New(rand.NewSource(...))`

Fixes #10988

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Signed-off-by: Thomas Graf <thomas@cilium.io>
joestringer pushed a commit that referenced this issue Jul 15, 2020
[ upstream commit fac5dde ]

Backport notes:
 Backported as requirement for b95650b ("etcd: Shuffle list of etcd endpoints")

The godoc comment for package `math/rand` states:

The default Source is safe for concurrent use by multiple goroutines, but
Sources created by NewSource are not.

The global source in `math/rand` would be concurrency-safe, but its
state may be interfered with from unrelated packages, e.g.

```
package a

func init() {
    math.Seed(time.Now().UnixNano()
}
```

and

```
package b

func init() {
    math.Seed(0)
}
```

Depending on the package load order, the PRNG state would be predictable
for all users (i.e.  always seeded using the fixed value 0), including
`package a`.

To fix this, provide a concurrency-safe wrapper around `math/rand.Rand`
in `pkg/rand` and use it to replace all existing users of
`math/rand.New(rand.NewSource(...))`

Fixes #10988

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Signed-off-by: Thomas Graf <thomas@cilium.io>
tklauser added a commit that referenced this issue Dec 8, 2023
Since Go 1.20 the top-level math/rand functions are auto-seeded by
default, see https://go.dev/doc/go1.20#library and
golang/go#54880. They also use the runtime's
lock-free fastrand64 function when 1. Seed has not been called and 2.
GODEBUG=randautoseed=0 is not used, see
golang/go#49892.

This allows to drop SafeRand and use the global source again. SafeRand
was introduced in commit fac5dde ("rand: add and use
concurrency-safe PRNG source") to fix #10988 by providing a
concurrency-safe PRNG source other than the global math/rand source
which couldn't be used at the time because it can't be interfered with
from unrelated packages by means of calling rand.Seed, see #10575.
rand.Seed is deprecated since GO 1.20 and per the paragraph above the
global source is seeded by default. This makes it unlikely that packages
would interfere with the global PRNG state anymore and the top-level
math/rand functions are safe to use again.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
tklauser added a commit that referenced this issue Dec 8, 2023
Since Go 1.20 the top-level math/rand functions are auto-seeded by
default, see https://go.dev/doc/go1.20#library and
golang/go#54880. They also use the runtime's
lock-free fastrand64 function when 1. Seed has not been called and 2.
GODEBUG=randautoseed=0 is not used, see
golang/go#49892.

This allows to drop SafeRand and use the global source again. SafeRand
was originally introduced in commit fac5dde ("rand: add and use
concurrency-safe PRNG source") to fix #10988 by providing a
concurrency-safe PRNG source other than the global math/rand source
which could be used at the time because it can't be interfered with from
unrelated packages by means of calling rand.Seed, see #10575. rand.Seed
is deprecated since Go 1.20 and per the paragraph above the global
source is seeded by default. This makes it unlikely that packages would
interfere with the global PRNG state anymore and the top-level math/rand
functions are safe to use again.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
tklauser added a commit that referenced this issue Dec 8, 2023
Since Go 1.20 the top-level math/rand functions are auto-seeded by
default, see https://go.dev/doc/go1.20#library and
golang/go#54880. They also use the runtime's
lock-free fastrand64 function when 1. Seed has not been called and 2.
GODEBUG=randautoseed=0 is not used, see
golang/go#49892.

This allows to drop SafeRand and use the global source again. SafeRand
was originally introduced in commit fac5dde ("rand: add and use
concurrency-safe PRNG source") to fix #10988 by providing a
concurrency-safe PRNG source other than the global math/rand source
which could be used at the time because it can't be interfered with from
unrelated packages by means of calling rand.Seed, see #10575. rand.Seed
is deprecated since Go 1.20 and per the paragraph above the global
source is seeded by default. This makes it unlikely that packages would
interfere with the global PRNG state anymore and the top-level math/rand
functions are safe to use again.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
tklauser added a commit that referenced this issue Dec 8, 2023
Since Go 1.20 the top-level math/rand functions are auto-seeded by
default, see https://go.dev/doc/go1.20#library and
golang/go#54880. They also use the runtime's
lock-free fastrand64 function when 1. Seed has not been called and 2.
GODEBUG=randautoseed=0 is not used, see
golang/go#49892.

This allows to drop SafeRand and use the global source again. SafeRand
was originally introduced in commit fac5dde ("rand: add and use
concurrency-safe PRNG source") to fix #10988 by providing a
concurrency-safe PRNG source other than the global math/rand source
which could be used at the time because it can't be interfered with from
unrelated packages by means of calling rand.Seed, see #10575. rand.Seed
is deprecated since Go 1.20 and per the paragraph above the global
source is seeded by default. This makes it unlikely that packages would
interfere with the global PRNG state anymore and the top-level math/rand
functions are safe to use again.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
tklauser added a commit that referenced this issue Dec 8, 2023
Since Go 1.20 the top-level math/rand functions are auto-seeded by
default, see https://go.dev/doc/go1.20#library and
golang/go#54880. They also use the runtime's
lock-free fastrand64 function when 1. Seed has not been called and 2.
GODEBUG=randautoseed=0 is not used, see
golang/go#49892.

This allows to drop SafeRand and use the global source again. SafeRand
was originally introduced in commit fac5dde ("rand: add and use
concurrency-safe PRNG source") to fix #10988 by providing a
concurrency-safe PRNG source other than the global math/rand source
which could be used at the time because it can't be interfered with from
unrelated packages by means of calling rand.Seed, see #10575. rand.Seed
is deprecated since Go 1.20 and per the paragraph above the global
source is seeded by default. This makes it unlikely that packages would
interfere with the global PRNG state anymore and the top-level math/rand
functions are safe to use again.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
tklauser added a commit that referenced this issue May 15, 2024
Since Go 1.22 the Go standard library provides the math/rand/v2 package
[^1]. Its global generator accessed by top-level functions is
unconditionally seeded using a true random value. Moreover, the new
package provides a way to construct PRNG sources which can be accessed
concurrently.

This allows to drop SafeRand and use the global source again where
applicable. SafeRand was originally introduced in commit fac5dde ("rand:
add and use concurrency-safe PRNG source") to fix #10988 by providing a
concurrency-safe PRNG source other than the global math/rand source
which could be used at the time because it can't be interfered with from
unrelated packages by means of calling rand.Seed, see #10575.

In some cases the global math/rand/v2 source cannot be used because unit
tests rely on seeding the PRNG source to a fixed value for deterministic
test runs. In these cases a math/rand/v2 source using fixed values is
used, like with SafeRand before.

[^1] https://go.dev/doc/go1.22#math_rand_v2

Signed-off-by: Tobias Klauser <tobias@cilium.io>
tklauser added a commit that referenced this issue May 15, 2024
Since Go 1.22 the Go standard library provides the math/rand/v2
package[^1]. Its global generator accessed by top-level functions is
unconditionally seeded using a true random value. Moreover, the new
package provides a way to construct PRNG sources which can be accessed
concurrently.

This allows to drop SafeRand and use the global source again where
applicable. SafeRand was originally introduced in commit fac5dde ("rand:
add and use concurrency-safe PRNG source") to fix #10988 by providing a
concurrency-safe PRNG source other than the global math/rand source
which could be used at the time because it can't be interfered with from
unrelated packages by means of calling rand.Seed, see #10575.

In some cases the global math/rand/v2 source cannot be used because unit
tests rely on seeding the PRNG source to a fixed value for deterministic
test runs. In these cases a math/rand/v2 source using fixed values is
used, like with SafeRand before.

[^1]: https://go.dev/doc/go1.22#math_rand_v2

Signed-off-by: Tobias Klauser <tobias@cilium.io>
tklauser added a commit that referenced this issue May 15, 2024
Since Go 1.22 the Go standard library provides the math/rand/v2
package[^1]. Its global generator accessed by top-level functions is
unconditionally seeded using a true random value. Moreover, the new
package provides a way to construct PRNG sources which can be accessed
concurrently.

This allows to drop SafeRand and use the global source again where
applicable. SafeRand was originally introduced in commit fac5dde ("rand:
add and use concurrency-safe PRNG source") to fix #10988 by providing a
concurrency-safe PRNG source other than the global math/rand source
which could be used at the time because it can't be interfered with from
unrelated packages by means of calling rand.Seed, see #10575.

In some cases the global math/rand/v2 source cannot be used because unit
tests rely on seeding the PRNG source to a fixed value for deterministic
test runs. In these cases a math/rand/v2 source using fixed values is
used, like with SafeRand before.

[^1]: https://go.dev/doc/go1.22#math_rand_v2

Signed-off-by: Tobias Klauser <tobias@cilium.io>
github-merge-queue bot pushed a commit that referenced this issue May 16, 2024
Since Go 1.22 the Go standard library provides the math/rand/v2
package[^1]. Its global generator accessed by top-level functions is
unconditionally seeded using a true random value. Moreover, the new
package provides a way to construct PRNG sources which can be accessed
concurrently.

This allows to drop SafeRand and use the global source again where
applicable. SafeRand was originally introduced in commit fac5dde ("rand:
add and use concurrency-safe PRNG source") to fix #10988 by providing a
concurrency-safe PRNG source other than the global math/rand source
which could be used at the time because it can't be interfered with from
unrelated packages by means of calling rand.Seed, see #10575.

In some cases the global math/rand/v2 source cannot be used because unit
tests rely on seeding the PRNG source to a fixed value for deterministic
test runs. In these cases a math/rand/v2 source using fixed values is
used, like with SafeRand before.

[^1]: https://go.dev/doc/go1.22#math_rand_v2

Signed-off-by: Tobias Klauser <tobias@cilium.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is a bug in the Cilium logic.
Projects
None yet
2 participants