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

sync: unchecked overflow in (*WaitGroup).Add #20687

Open
bcmills opened this issue Jun 15, 2017 · 5 comments

Comments

@bcmills
Copy link
Member

commented Jun 15, 2017

Thinking about #20678 (and #19624) made me wonder what other unchecked overflows are lurking in the standard library. I've found another one, but I don't have real-world code that triggers it. (I'm filing this issue just for reference.)

(*sync.WaitGroup).Add accepts an int parameter, so it is reasonable for users to expect that they cannot make Add calls summing to more than the maximum possible int. However, in practice WaitGroup only supports Add calls that sum to within math.MaxInt32.

Add currently detects (and panics on) overflows that happen to land in the negative half of the int32 space, but fails to detect other overflows entirely.

Add should be fixed to reliably panic on all overflows. It should ideally support the full positive int range, but if that isn't feasible the supported range should be made clear in the documentation.

$ go version
go version devel +0a0e45d5c6 Thu May 25 17:57:21 2017 -0400 linux/amd64

wgover/wgover.go:

package main

import (
	"fmt"
	"math"
	"sync"
	"sync/atomic"
)

func main() {
	var wg sync.WaitGroup
	var n int64

	const maxInt = int(^uint(0) >> 1)
	iters := 0x40000000
	if int64(maxInt) > math.MaxInt32 {
		iters <<= 2
	}

	fmt.Printf("looping for %d iterations\n", iters)
	wg.Add(iters)
	atomic.AddInt64(&n, int64(iters))
	go func() {
		for {
			wg.Done()
			if atomic.AddInt64(&n, -1) == 0 {
				break
			}
		}
	}()
	wg.Wait()

	fmt.Printf("done with %d iterations remaining\n", atomic.LoadInt64(&n))
}

expected (with very long running time):

$ go run wgover/wgover.go
looping for 4294967296 iterations
done with 0 iterations remaining

actual:

$ go run wgover/wgover.go
looping for 4294967296 iterations
done with 4294967296 iterations remaining
@gopherbot

This comment has been minimized.

Copy link

commented Nov 29, 2017

Change https://golang.org/cl/80835 mentions this issue: sync: detect WaitGroup counter overflow

@urld

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

Is the discussion in https://golang.org/cl/80835 still up to date?
If the conclusion is still to use runtime.throw to avoid inconsistent states and performance impacts (based on https://golang.org/cl/31359), i could submit a change.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

@urld It's a worth a try though I don't know for sure what everyone will think. Thanks.

@gopherbot

This comment has been minimized.

Copy link

commented Oct 9, 2018

Change https://golang.org/cl/140937 mentions this issue: sync: detect WaitGroup counter overflow

@urld

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2018

As already mentioned in the abandoned CL, I'm not sure anymore, if it is possible to detect overflows in all cases, without turning Add into a CAS loop.

Maybe there should be a variation of sync/atomic.AddUint64, which checks for overflows and reports them:

func AddUintptr(addr *uintptr, delta uintptr) (new uintptr, overflowed bool)

If an overflow is reported during counter modification, the program could be terminated by throw since the counter may be in an inconsistent state due to concurrent modifications.
The case in which the counter becomes negative could still cause a panic which can be recovered from (if no overflow was involved).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.