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

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

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

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

bcmills opened this issue Jun 15, 2017 · 5 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@bcmills
Copy link
Contributor

bcmills 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
@bradfitz bradfitz added this to the Go1.10 milestone Jun 15, 2017
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 15, 2017
@bcmills bcmills mentioned this issue Jun 20, 2017
Closed
@gopherbot
Copy link

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

@rsc rsc modified the milestones: Go1.10, Go1.11 Dec 4, 2017
@resouer resouer mentioned this issue Apr 29, 2018
Merged
@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Unplanned Jun 29, 2018
@urld
Copy link
Contributor

urld 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
Copy link
Contributor

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

@gopherbot
Copy link

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

@urld
Copy link
Contributor

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

@bcmills bcmills mentioned this issue Jun 18, 2020
Open
@danxmoran danxmoran mentioned this issue Nov 23, 2020
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@bradfitz @rsc @ianlancetaylor @bcmills @gopherbot @urld