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

when invoke allocateAdditionalMemory() , if the free space is 128 bytes, bug occurs #253

Open
ekkoxx opened this issue Nov 13, 2020 · 4 comments
Labels

Comments

@ekkoxx
Copy link

ekkoxx commented Nov 13, 2020

The allocateAdditionalMemory method in bigcache/queue/bytes_queue.go file has the following code:

		if q.tail <= q.head {
			if q.tail != q.head {
				headerEntrySize := getUvarintSize(uint32(q.head - q.tail))
				emptyBlobLen := q.head - q.tail - headerEntrySize
				q.push(make([]byte, emptyBlobLen), emptyBlobLen)
			}

			q.head = leftMarginIndex
			q.tail = q.rightMargin
		}

When head - tail == 128, the q.push() function actually only stores 127 bytes, one less byte.
A bug occurs when you call the Pop function。
See the following unit test for details:

func TestAllocateAdditionalSpaceForInsufficientFreeFragmentedSpaceWhereTailIsBeforeHead(t *testing.T) {
	t.Parallel()

	// given
	queue := NewBytesQueue(200, 0, false)

	// when 
	queue.Push(blob('a', 30)) //  header + entry + left margin = 32 bytes
	queue.Push(blob('b', 127)) // 32 + 127 + 1 = 160 bytes
	queue.Push(blob('c', 20)) //  160 + 20 + 1 = 181
	queue.Pop()               // space freed at the beginning
	queue.Pop()               //
	queue.Push(blob('d', 30)) // 31 bytes used at the beginning, tail pointer is before head pointer, now free space is 128 bytes
	queue.Push(blob('e', 160)) // invoke allocateAdditionalMemory but fill 127 bytes free space (It should be 128 bytes, but 127 are filled, leaving one byte unfilled)

	// then
	//assertEqual(t, 400, queue.Capacity())
	//assertEqual(t, blob('d', 30), pop(queue))
	//assertEqual(t, blob(0, 127), pop(queue))    //error
	//assertEqual(t, blob('c', 20), pop(queue))   //The data is not expected
	//assertEqual(t, blob('e', 160), pop(queue))
}
@janisz
Copy link
Collaborator

janisz commented Nov 13, 2020

Refs #251

@ekkoxx
Copy link
Author

ekkoxx commented Nov 15, 2020

If head-tail == 129, I think there is no way to deal with it. Because no length is 129 byte after being encoded with varint.

@janisz
Copy link
Collaborator

janisz commented Nov 16, 2020

So it looks like the only way to fix this is to revert #207 and not use Varint

@panmari
Copy link
Contributor

panmari commented Sep 3, 2021

I think I'm also seeing this problem for my use case. Are there any blockers that prevent merging @Fabianexe 's change? The test included verifies that this fixes the problem.

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

No branches or pull requests

3 participants