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

Fix integer overflow in more offset calculations #224

Merged
2 commits merged into from May 21, 2018
Merged

Conversation

ghost
Copy link

@ghost ghost commented May 18, 2018

Fixing more cases of integer overflow similar to those fixed in #222.

  • identify potential overflow and upcast to long

    • idiom 1: ordinal * bitsPerX where both are int; can be bucket or element in addition to ordinal
    • idiom 2: ordinal + N where ordinal is assigned to a long or passed as an argument that accepts longs
      • doesn't include BitSet which only works with int indices; this is one of the reasons ordinals are currently limited to integers
  • standardize style on (long)ordinal * bitsPerX to make it easier to spot the common pattern and notice missing upcasts

    • replace: (long)bitsPerX * ordinal, bitsPerX * (long)ordinal, or (long)bitsPerX * (long)ordinal
    • replace: ordinal * (long)bitsPerX or (long)ordinal * (long)bitsPerX
    • avoid redundant upcast; in some cases ordinal is already a long temporary variable or passed as long to a private helper; public APIs should only pass ordinal as int to avoid confusion
  • review com.netflix.hollow.core.read.engine

    • review list.*
    • review set.*
    • review map.*
    • review object.*

@ghost ghost force-pushed the fix/offset-overflow branch 2 times, most recently from 31086f4 to 299bae9 Compare May 18, 2018 02:39
@ghost ghost self-assigned this May 18, 2018
@ghost ghost requested review from akhaku and kineshsatiya May 18, 2018 02:45
@ghost
Copy link
Author

ghost commented May 18, 2018

/cc @adamkeyser @manuelq

@ghost ghost changed the title Fix integer overflow in offset calculations Fix integer overflow in more offset calculations May 18, 2018
@ghost ghost force-pushed the fix/offset-overflow branch from 299bae9 to 07a9401 Compare May 18, 2018 03:05
@ghost ghost force-pushed the fix/offset-overflow branch from 07a9401 to cda75b7 Compare May 18, 2018 03:08
Copy link
Member

@rpalcolea rpalcolea left a comment

Choose a reason for hiding this comment

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

This looks great! hopefully we can get a release soon :)

@manuelq
Copy link

manuelq commented May 18, 2018

@toolbear
We tested this branch out locally with this test below added to HollowListFastDeltaTest and it works great. This takes about 2 minutes to run locally so not sure if this could be added to the suite but it's a good verification.

@Test
    public void testLarge() throws IOException {
        int listOrdinal = 0;        //2147483647 MAX INT
        int totalRecords = 0;       // WHY 79536431 because bitsPerListPointer = 27 so MaxINT/27
        for(int ordinal = 0; ordinal <= 79536431;ordinal++){
            addRecord(listOrdinal++);
            totalRecords++;

        }
        //Just add another 100k
        System.out.println(totalRecords);
        for(int ordinal = 0; ordinal <= 100000;ordinal++){
            addRecord(listOrdinal++);
            totalRecords++;
        }
        System.out.println(totalRecords);
        roundTripSnapshot();

        HollowListTypeReadState typeState = (HollowListTypeReadState) readStateEngine.getTypeState("TestList");

        assertList(typeState, 14, 14);

        assertList(typeState, 79636430, 79636430);
        HollowChecksum checksum = typeState.getChecksum(readStateEngine.getSchema("TestList"));
        checksum.intValue();
    }

@manuelq
Copy link

manuelq commented May 18, 2018

On the master branch the test above throws

java.lang.ArrayIndexOutOfBoundsException: -130908

	at com.netflix.hollow.core.memory.encoding.FixedLengthElementArray.getElementValue(FixedLengthElementArray.java:93)
	at com.netflix.hollow.core.memory.encoding.FixedLengthElementArray.getElementValue(FixedLengthElementArray.java:84)
	at com.netflix.hollow.core.read.engine.list.HollowListTypeReadStateShard.size(HollowListTypeReadStateShard.java:73)
	at com.netflix.hollow.core.read.engine.list.HollowListTypeReadState.size(HollowListTypeReadState.java:143)
	at com.netflix.hollow.core.read.iterator.HollowListOrdinalIterator.<init>(HollowListOrdinalIterator.java:32)
	at com.netflix.hollow.core.read.engine.list.HollowListTypeReadState.ordinalIterator(HollowListTypeReadState.java:150)
	at com.netflix.hollow.core.read.list.HollowListFastDeltaTest.assertList(HollowListFastDeltaTest.java:161)
	at com.netflix.hollow.core.read.list.HollowListFastDeltaTest.testLarge(HollowListFastDeltaTest.java:145)```

@ghost ghost merged commit b1329ff into master May 21, 2018
@ghost ghost deleted the fix/offset-overflow branch October 5, 2018 00:21
Sunjeet pushed a commit that referenced this pull request Apr 10, 2019
…_test to master

* commit 'b637bb0c0ef9d6551bfc3f4743defff27b7c3dc5':
  Add hostname to smoke test data track
This pull request was closed.
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.

None yet

3 participants