Skip to content

Conversation

hedisam
Copy link

@hedisam hedisam commented Apr 23, 2021

RingBuffer with a size of 1 will have a mask of 0 which uncovers a bug in the algorithm since the mask value must only contain ones (left padded with zeros) if it's represented in its binary format (e.g. '0111', '0011 1111', etc.). So we should not allow having RingBuffer queues with a size of 1. We can simply fix this by incrementing the size inside the RingBuffer's init method.

With a full RingBuffer of size 1, insert operations replace the existing item in the queue and then puts the RingBuffer into an invalid state.

… 1 due to occuring replacements having a full queue with a size of one
@aviary-wf
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

@hedisam
Copy link
Author

hedisam commented Apr 23, 2021

You can see the bug in action by running this unit test:

func TestRingInsertSizeOne(t *testing.T) {
	rb := NewRingBuffer(1)
	assert.Equal(t, uint64(1), rb.Cap())

	err := rb.Put("Hello")
	if !assert.Nil(t, err) {
		return
	}

	ok, err := rb.Offer("Hello, Again.")
	if !assert.Nil(t, err) {
		return
	}
	assert.False(t, ok)

        // rb.Get() blocks the routine since our RingBuffer's state becomes invalid 
	result, err := rb.Poll(time.Millisecond * 100)
	if !assert.Nil(t, err) {
		return
	}
	if !assert.NotNil(t, result) {
		return
	}
	assert.Equal(t, "Hello", result)
}

// single item in the ring buffer gets replaced with every insert operation which also makes
// rb.Get() to block since the ring buffer's state becomes invalid despite having a full queue.
size = 2
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very interesting. You have found a bug in the roundUp method that occurs when size <= 1. I propose that we fix that method instead. I also see that it is copied in a few different places so we should move it to a single location.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants