bigcache icon indicating copy to clipboard operation
bigcache copied to clipboard

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

Open ekkoxx opened this issue 3 years ago • 4 comments

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))
}

ekkoxx avatar Nov 13 '20 10:11 ekkoxx

Refs #251

janisz avatar Nov 13 '20 10:11 janisz

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.

ekkoxx avatar Nov 15 '20 09:11 ekkoxx

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

janisz avatar Nov 16 '20 16:11 janisz

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.

panmari avatar Sep 03 '21 08:09 panmari