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: dequeue newSize may be overflow on 32bit system #44532

Open
math345 opened this issue Feb 23, 2021 · 2 comments
Open

sync: dequeue newSize may be overflow on 32bit system #44532

math345 opened this issue Feb 23, 2021 · 2 comments
Labels
NeedsInvestigation
Milestone

Comments

@math345
Copy link

@math345 math345 commented Feb 23, 2021

const dequeueBits = 32

// dequeueLimit is the maximum size of a poolDequeue.
//
// This must be at most (1<<dequeueBits)/2 because detecting fullness
// depends on wrapping around the ring buffer without wrapping around
// the index. We divide by 4 so this fits in an int on 32-bit.
const dequeueLimit = (1 << dequeueBits) / 4

func (c *poolChain) pushHead(val interface{}) {
	d := c.head
	if d == nil {
		// Initialize the chain.
		const initSize = 8 // Must be a power of 2
		d = new(poolChainElt)
		d.vals = make([]eface, initSize)
		c.head = d
		storePoolChainElt(&c.tail, d)
	}

	if d.pushHead(val) {
		return
	}

	// The current dequeue is full. Allocate a new one of twice
	// the size.
	newSize := len(d.vals) * 2
	if newSize >= dequeueLimit {
		// Can't make it any bigger.
		newSize = dequeueLimit
	}

	d2 := &poolChainElt{prev: d}
	d2.vals = make([]eface, newSize)
	c.head = d2
	storePoolChainElt(&d.next, d2)
	d2.pushHead(val)
}

len(d.vals) return type is int which on 32 bit system is 4 bytes. When d.vals reachs the max size 2^30, then newSize will be 2^31. But 2^31 is greater than the max int32 2^31 -1 , and if condition will test fail .

newSize := len(d.vals) * 2
@bcmills
Copy link
Member

@bcmills bcmills commented Feb 23, 2021

Neat find!

Fortunately, I suspect your program would always crash due to address space exhaustion before you can actually hit the overflow. The elements in d.vals have type eface, which on a 32-bit platform is 8 bytes. A slice of 230 8-byte elements would be 233 bytes consumed by the slice's backing array, but since we're assuming a 32-bit platform we only have 232 bytes of address space to work with.

So AFAICT this overflow is only possible on platforms with a 32-bit int but an address space much larger than 32 bits, and to my knowledge we have no such platform today.

That said, that rationale should at least merit a comment in the code...

@cagedmantis cagedmantis changed the title sync/poolqueue.go dequeue newSize may be overflow on 32bit system sync: dequeue newSize may be overflow on 32bit system Feb 23, 2021
@cagedmantis cagedmantis added the NeedsInvestigation label Feb 23, 2021
@cagedmantis cagedmantis added this to the Backlog milestone Feb 23, 2021
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Feb 23, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

3 participants