KAFKA-9419: Integer Overflow Possible with CircularIterator by belugabehr · Pull Request #7950 · apache/kafka

belugabehr · 2020-01-13T21:16:38Z

The CircularIterator class uses a wrapping index-based approach to iterate over a list. This can be a performance problem O(n^2) for a LinkedList. Also, the index counter itself is never reset, a modulo is applied to it for every list access. At some point, it may be possible that the index counter overflows to a negative value and therefore may cause a negative index read and an ArrayIndexOutOfBoundsException.

I propose changing this implementation to avoid these two scenarios. Use the Collection Iterator classes to avoid using an index counter and it avoids having to seek to the correct index every time, this avoiding the LinkedList performance issue.

I have added unit tests to validate the new implementation.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

rondagostino

Thanks for the PR. Agree with the intent to eliminate index-based access (not just for the overflow problem, but also for the O(N) performance on some lists). Need to support null elements with the collection (and test it).

rondagostino · 2020-01-14T18:46:17Z

Why change the logic since the answer is always true?

Astute observation. So, technically speaking, it is possible to not return true. Imagine the following code....

List a = new ArrayList<>(Arrays.asList("A")); Iterator it = new CircularIterator(a); System.out.println(it.next()); a.clear(); System.out.println(it.next());

The Iterator is re-created on the second call to next() so this is a legal option. Now, not sure it would actually happen, but it's possible.

The constructor doesn't accept an empty list, but it doesn't copy it (or, better, if we were using persistent data structures... but alas no). The class isn't thread-safe, and iterators that the code constructs and uses are potentially fail-safe or fail-fast if the collection is modified. And the remove() method of this class throws UnsupportedOperationException.

All in all, I think the best thing is to clearly document that the class does not support modifications to the passed-in collection and always return true here (technically !isEmpty() covers all cases, but since the constructor doesn't accept an empty list the answer would always be true).

belugabehr · 2020-01-14T20:38:18Z

@rondagostino Changes applied. Thanks.

rondagostino

I think you can get rid of hasPeek and simply keep the current peek value at all times. You can create an advance() method that you call in the constructor and in next(). Then you simply return the peek value in peek() and next() (calling advance() in next() first, of course). Then all the logic is in one place. At last I think that will work. (actually, call advance() at the end of next())

rondagostino · 2020-01-14T22:53:03Z

Should simply return true here, right? As written now it is adjusting internal state, and it is best to isolate state manipulation to as few places as possible.

belugabehr · 2020-01-15T01:07:05Z

Ok. I think the class is trivial enough that it won't throw folks for a loop, but I can change the next() method if you so require. The constructor will load the first value and store it in 'peek' (currently named). At the end of each next() call, it will load the next value, rotating Iterators if required, into 'peek'. The state change is an artifact of my trying to gracefully handle manipulation of the underlying collection in hasNext(). It can be reduced to a single boolean. Give me a couple of days. I had a marathon of Kafka PRs and am now spending a couple of days with people, not computers. :)

…

On Tue, Jan 14, 2020, 6:10 PM Ron Dagostino ***@***.***> wrote: ***@***.**** commented on this pull request. I think you can get rid of hasPeek and simply keep the current peek value at all times. You can create an advance() method that you call in the constructor and in next(). Then you simply return the peek value in peek() and next() (calling advance() in next() first, of course). Then all the logic is in one place. At last I think that will work. ------------------------------ In clients/src/main/java/org/apache/kafka/common/utils/CircularIterator.java <#7950 (comment)>: > @OverRide public boolean hasNext() { + if (this.hasPeek) { Should simply return true here, right? As written now it is adjusting internal state, and it is best to isolate state manipulation to as few places as possible. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7950?email_source=notifications&email_token=AC766E5YXIN6HCQJIL3RJN3Q5ZA4ZA5CNFSM4KGI57QKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCRYCDLY#pullrequestreview-342892975>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC766E44S56RRXLYWWFPIA3Q5ZA4ZANCNFSM4KGI57QA> .

rondagostino · 2020-01-15T15:56:46Z

I just realized that changing from an index-based approach to an iterator one changes the contract of this class -- currently it is possible to share the list and delete from it while it is being used here because there are no iterators involved. This would no longer be the case if we go to the proposed iterator-based approach. I did a quick search and found that it isn't a problem with the 2 cases where this class is currently being used (RoundRobinAssignor and EagerAsignor). Changes in the org.apache.kafka.common.utils package do not require a KIP as well, so it looks like there is no problem here. So this comment serves as documentation that we considered this issue.

…sNext()

belugabehr · 2020-03-02T16:49:16Z

@rondagostino I made the requested changes. Please review.

rondagostino

Implementation LGTM; 1 comment on the test.

rondagostino · 2020-03-02T19:13:09Z

+    @Test()
+    public void testCycleCollection() {
+        final CircularIterator<String> it = new CircularIterator<>(Arrays.asList("A", "B", "C"));
+
+        assertTrue(it.hasNext());
+        assertEquals("A", it.next());
+        assertTrue(it.hasNext());
+        assertEquals("B", it.next());
+        assertTrue(it.hasNext());
+        assertEquals("C", it.next());
+        assertTrue(it.hasNext());
+        assertEquals("A", it.next());
+        assertTrue(it.hasNext());
+    }
+
+    @Test()
+    public void testPeekCollection() {
+        final CircularIterator<String> it = new CircularIterator<>(Arrays.asList("A", "B", "C"));
+
+        assertTrue(it.hasNext());
+        assertEquals("A", it.peek());
+        assertTrue(it.hasNext());
+        assertEquals("A", it.peek());
+        assertTrue(it.hasNext());
+        assertEquals("A", it.next());
+
+        assertEquals("B", it.next());
+        assertTrue(it.hasNext());
+        assertEquals("C", it.next());
+        assertTrue(it.hasNext());
+        assertEquals("A", it.next());
+        assertTrue(it.hasNext());
+    }
+
+    @Test()
+    public void testPeekCollectionNullValue() {
+        final CircularIterator<String> it = new CircularIterator<>(Arrays.asList("A", null, "C"));
+
+        assertTrue(it.hasNext());
+        assertEquals("A", it.peek());
+        assertTrue(it.hasNext());
+        assertEquals("A", it.next());
+
+        assertTrue(it.hasNext());
+        assertEquals(null, it.peek());
+        assertTrue(it.hasNext());
+        assertEquals(null, it.peek());
+        assertTrue(it.hasNext());
+        assertEquals(null, it.next());
+
+        assertTrue(it.hasNext());
+        assertEquals("C", it.next());
+    }


Could combine these into a single testCycleCollection() method if you put a null value in the list (e.g. "A", null, "C") and for every one of the 4 positions (0-2 and cycling back to 0) you also check the peek() value. I think it would be clearer compared to what you have currently since the last 2 methods you have now are a bit haphazard.

belugabehr · 2020-03-03T19:01:25Z

@rondagostino I made the requested changes to the unit test. Thanks.

rondagostino

LGTM, thanks for the PR!

belugabehr · 2020-03-04T03:49:39Z

@rondagostino Thanks for all the reviews!

belugabehr · 2020-03-15T19:53:37Z

@rondagostino Possible to get this merged? Thanks!

kkonstantine · 2020-03-22T06:36:32Z

Thanks for the PR @belugabehr and the reviews @rondagostino

I took a quick pass and the implementation looks overall. I'll take a closer look, since I believe it makes sense to backport. Because of that it's better to wait for the outstanding 2.5.0 to be released.

In the meantime, it's definitely a good idea to add a description of the change for any non-trivial PRs.

kkonstantine · 2020-03-22T06:36:40Z

ok to test

belugabehr · 2020-04-16T14:09:48Z

@kkonstantine Can you please kick of another test so we can get this wrapped up? :)

Thanks.

kkonstantine

Almost there. Added a few final minor comments.

kkonstantine · 2020-05-06T18:03:07Z

retest this please

belugabehr · 2020-05-06T18:06:59Z

@kkonstantine I appreciate your help getting this over the finish line

kkonstantine · 2020-05-06T18:16:46Z

Definitely. It's a nice fix. Thanks for contributing it @belugabehr

kkonstantine · 2020-05-06T22:58:22Z

jdk 8: stellar
jdk 11: 2 streams eos test failures: unrelated
jkd 14: 1 more unrelated failure (kafka.api.TransactionsTest.testBumpTransactionalEpoch)

Given this outcome, I'm merging.

kkonstantine

LGTM

rondagostino reviewed Jan 14, 2020

View reviewed changes

belugabehr added 4 commits March 2, 2020 11:02

KAFKA-9419: Integer Overflow Possible with CircularIterator

bf49fbb

Added JavaDoc. Support null values in the underlying collection

417ea8b

Always return true for hasNext(). Add more JavaDoc

f8998d2

Use an advance method to load next value and always return true in ha…

b2a2349

…sNext()

belugabehr force-pushed the KAFKA-9419 branch from 82576fd to b2a2349 Compare March 2, 2020 16:38

rondagostino reviewed Mar 2, 2020

View reviewed changes

Simplify test suite

4520f19

rondagostino reviewed Mar 3, 2020

View reviewed changes

kkonstantine added connect consumer labels Mar 22, 2020

kkonstantine reviewed Apr 30, 2020

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/common/utils/CircularIterator.java Outdated

Comment thread clients/src/main/java/org/apache/kafka/common/utils/CircularIterator.java Outdated

Comment thread clients/src/test/java/org/apache/kafka/common/utils/CircularIteratorTest.java Outdated

Use assertThrows in tests and remove redundant 'this' identifier

01eb7d1

kkonstantine approved these changes May 6, 2020

View reviewed changes

kkonstantine merged commit 074ab2e into apache:trunk May 6, 2020

Uh oh!

Conversation

belugabehr commented Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

rondagostino left a comment

Choose a reason for hiding this comment

Uh oh!

rondagostino Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

belugabehr Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

rondagostino Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

belugabehr commented Jan 14, 2020

Uh oh!

rondagostino left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rondagostino Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

belugabehr commented Jan 15, 2020 via email

Uh oh!

rondagostino commented Jan 15, 2020

Uh oh!

belugabehr commented Mar 2, 2020

Uh oh!

rondagostino left a comment

Choose a reason for hiding this comment

Uh oh!

rondagostino Mar 2, 2020

Choose a reason for hiding this comment

Uh oh!

belugabehr commented Mar 3, 2020

Uh oh!

rondagostino left a comment

Choose a reason for hiding this comment

Uh oh!

belugabehr commented Mar 4, 2020

Uh oh!

belugabehr commented Mar 15, 2020

Uh oh!

kkonstantine commented Mar 22, 2020

Uh oh!

kkonstantine commented Mar 22, 2020

Uh oh!

belugabehr commented Apr 16, 2020

Uh oh!

kkonstantine left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kkonstantine commented May 6, 2020

Uh oh!

belugabehr commented May 6, 2020

Uh oh!

kkonstantine commented May 6, 2020

Uh oh!

kkonstantine commented May 6, 2020

Uh oh!

kkonstantine left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

belugabehr commented Jan 13, 2020 •

edited

Loading

rondagostino left a comment •

edited

Loading