java.lang.ArrayIndexOutOfBoundsException

[mos8502]

We have started seeing this causing a crash using V 4.3.0 on Android P in production. Unfortunately I have yet to reproduce this issue myself.

Fatal Exception: java.lang.ArrayIndexOutOfBoundsException: length=16; regionStart=0; regionLength=-123478
       at java.util.Arrays.checkOffsetAndCount(Arrays.java:135)
       at libcore.io.IoBridge.read(IoBridge.java:496)
       at java.io.RandomAccessFile.readBytes(RandomAccessFile.java:385)
       at java.io.RandomAccessFile.read(RandomAccessFile.java:414)
       at java.io.RandomAccessFile.readFully(RandomAccessFile.java:473)
       at com.segment.analytics.QueueFile.ringRead(QueueFile.java:268)
       at com.segment.analytics.QueueFile.readElement(QueueFile.java:181)
       at com.segment.analytics.QueueFile.readHeader(QueueFile.java:159)
       at com.segment.analytics.QueueFile.<init>(QueueFile.java:118)
       at com.segment.analytics.SegmentIntegration.createQueueFile(SegmentIntegration.java:134)
       at com.segment.analytics.SegmentIntegration.create(SegmentIntegration.java:160)
       at com.segment.analytics.SegmentIntegration$1.create(SegmentIntegration.java:51)
       at com.segment.analytics.Analytics.performInitializeIntegrations(Analytics.java:1447)
       at com.segment.analytics.Analytics$2$1.run(Analytics.java:275)
       at android.os.Handler.handleCallback(Handler.java:873)
       at android.os.Handler.dispatchMessage(Handler.java:99)
       at android.os.Looper.loop(Looper.java:193)
       at android.app.ActivityThread.main(ActivityThread.java:6669)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)

[f2prateek]

Interesting, we haven't seen this before. We could expand our check when we create the file (https://github.com/segmentio/analytics-android/blob/master/analytics/src/main/java/com/segment/analytics/SegmentIntegration.java#L132-L142). Catching more general exceptions could lead to other bugs though, so it would be good to understand why this is happening.

[mos8502]

@f2prateek I agree that catching the exception would not be a good approach. I will try to spend a bit more time to see if I can reproduce it myself

[seaplain]

We have been experiencing this crash for a while now, and in quite large volumes. It also seems to be the same issue as what is raised here: #627

I dug further into the cause and have a suggestion for a more targeted fix. Here are the conclusions I was able to draw.

---

RandomAccessFile.read will throw an IndexOutOfBoundsException in the following cases:

* @exception  IndexOutOfBoundsException If {@code off} is negative,      
* {@code len} is negative, or {@code len} is greater than      
* {@code b.length - off} 

I worked backwards to figure out if any of these cases are true starting with QueueFile.readElement

• Here we can see that off is set to 0.
• We can also see that b.length is 16 because the array is initialized in QueueFile and never reassigned.

This means that either len is negative or len is greater than 16.

---

If we step down into QueueFile.ringRead we can see that len is set from beforeEof via:

int beforeEof = fileLength - position;

We know fileLength is positive because fileLength <= 0 is checked way back up the chain in QueueFile.readHeader. And we know that position is not 0 because that is checked in readElement.

We also know that position + count <= fileLength is false where count is set to 4 (Element.HEADER_LENGTH) as that is checked earlier in the function.

So position is greater than or equal to fileLength - 3.

Which means it is possible to get into a situation where len is negative as position can be greater than fileLength.

But it is important to note that this isn’t easy to achieve. As wrapPosition limits the size of position. This means that the original position must be ~double the fileLength for this crash to occur.

---

position is derived from the file contents in QueueFile.readHeader. For us it is firstOffset that seems to be causing the problem, but lastOffset could also be invalid and we just aren't reaching that line.

int firstOffset = readInt(buffer, 8);
int lastOffset = readInt(buffer, 12);
first = readElement(firstOffset);
last = readElement(lastOffset);

When I changed the contents of buffer manually in the debugger I was able to see the crash:

java.lang.ArrayIndexOutOfBoundsException: length=16; regionStart=0; regionLength=-1824

In this case I set firstOffset to 10,000.

---

I attempted to see if there were any obvious cases where such a large value was sneaking into the header. But I didn't have much luck. Outside of writeHeader all other updates to the header values are to set it back to 0. And writeHeader itself is called in add, remove and clear with loads of checks around them that don't appear to have any gaps.

The most likely culprit would be something in add but as the positions are updated according to the size of the elements being written / removed it is hard to make any conclusions.

---

Ultimately I think that further digging is likely burnt effort at this point. Considering it only seems to occur on Android 9, there is probably something suspect happening at the OS level and not anything wrong with the actual write logic.

It seems best to expand the corrupt file checks around fileLength to include the other header values. This would allow the check in SegmentIntegration.createQueueFile to handle this specific case without becoming too general.

I.e. update readHeader to be something like:

private void readHeader() throws IOException {
    raf.seek(0);
    raf.readFully(buffer);

    fileLength = readInt(buffer, 0);
    elementCount = readInt(buffer, 4);
    int firstOffset = readInt(buffer, 8);
    int lastOffset = readInt(buffer, 12);

    if (fileLength > raf.length()) {
      throw new IOException("File is truncated. Expected length: " + fileLength + ", Actual length: " + raf.length());
    } else if (fileLength <= 0) {
      throw new IOException("File is corrupt; length stored in header (" + fileLength + ") is invalid.");
    } else if (fileLength <= wrapPosition(firstOffset)) {
      throw new IOException("File is corrupt; first position stored in header (" + firstOffset + ") is invalid.");
    } else if (fileLength <= wrapPosition(lastOffset)) {
      throw new IOException("File is corrupt; last position stored in header (" + lastOffset + ") is invalid.");
    }
    first = readElement(firstOffset);
    last = readElement(lastOffset);
  }

I am happy to raise a PR: @f2prateek are you still the right person to talk to about this?

[aultimus]

Hi, @seaplain , thanks for your debugging and detailed comment. @f2prateek is no longer actively maintaining this repo. However, I am now doing some maintenance work for Segment on this repo though I am new to this codebase.

A PR would be most welcome and I will review.
Is it possible to add a unit test case that captures this code change?
Is this error occuring due to the queuefile becoming corrupted?

[seaplain]

Thanks @aultimus I'll put up a PR asap and aim to create some test cases for it too.

I am not certain why this could happen. I narrowed it down to either an external corruption of the file or some edge case where adding / removing to the QueueFile can leave it in an invalid state. I believe it is the former because we only see this crash on Android 9 devices. But ultimately yes for this to happen the values stored in the file must be invalid.

I'm not confident I have enough knowledge of the codebase and problem-space to debug any further than that. I do have a device that matches the crashes we have seen but I wasn't able to reproduce it myself.

[aultimus]

Hi, could you please try release 4.5.0-beta.2 which should address this issue. I am closing this issue, please re-open if the problem persists

https://github.com/segmentio/analytics-android/releases/tag/4.5.0-beta.2