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

java.lang.ArrayIndexOutOfBoundsException at CBORGenerator.java:548 #62

Closed
mlmitch opened this issue Mar 15, 2017 · 6 comments
Closed

java.lang.ArrayIndexOutOfBoundsException at CBORGenerator.java:548 #62

mlmitch opened this issue Mar 15, 2017 · 6 comments

Comments

@mlmitch
Copy link

mlmitch commented Mar 15, 2017

I am encountering index out of bound exceptions at CBORGenerator.java:548 when performing serialization with a new ObjectMapper(new CBORFactory()) instance. I haven't done a full investigation of the cause since calls to CBORGenerator::writeStartObject are very stateful. Here is the offending function.

@Override
// since 2.8
public final void writeStartObject(Object forValue) throws IOException {
    _verifyValueWrite("start an object");
    JsonWriteContext ctxt = _writeContext.createChildObjectContext();
    _writeContext = ctxt;
    if (forValue != null) {
        ctxt.setCurrentValue(forValue);
    }
    if (_elementCountsPtr > 0) {
        _elementCounts[_elementCountsPtr++] = _currentRemainingElements;
    }
    _currentRemainingElements = INDEFINITE_LENGTH;
    _writeByte(BYTE_OBJECT_INDEFINITE);
}

An example error producing call has _elementCountsPtr = 11 and _elementCounts.length = 10. My guess at a solution and my current workaround is to add a safety check like in CBORGenerator::writeStartArray. Here is that function.

@Override
public void writeStartArray(int elementsToWrite) throws IOException {
    _verifyValueWrite("start an array");
    _writeContext = _writeContext.createChildArrayContext();
    if (_elementCounts.length == _elementCountsPtr) { // initially, as well as if full
        _elementCounts = Arrays.copyOf(_elementCounts, _elementCounts.length+10);
    }
    _elementCounts[_elementCountsPtr++] = _currentRemainingElements;
    _currentRemainingElements = elementsToWrite;
    _writeLengthMarker(PREFIX_TYPE_ARRAY, elementsToWrite);
}

As you can see, the function resizes the _elementCounts array when required. I don't know what kind of coding styles you want to employ, but I would change the offending section in every writeStartArray and writeStartObject overload to read:

    if (_elementCounts.length <= _elementCountsPtr) { // less than or equal to catch more bad cases
        //use _elementCountsPtr for new size to guarantee the array is big enough for this call
        _elementCounts = Arrays.copyOf(_elementCounts, _elementCountsPtr+10); 
    }
    if (_elementCountsPtr > 0) { //guard for negative indexes
        _elementCounts[_elementCountsPtr++] = _currentRemainingElements;
    }
    

Sorry I couldn't be more helpful with regard to why it is happening. Though I'd be happy to make a pull request with my described solution. Let me know what you think.

@mlmitch
Copy link
Author

mlmitch commented Mar 15, 2017

It looks like

if (_elementCounts.length <= _elementCountsPtr) { // less than or equal to catch more bad cases
    //use _elementCountsPtr for new size to guarantee the array is big enough for this call
    _elementCounts = Arrays.copyOf(_elementCounts, _elementCountsPtr+10);  
}
if (_elementCountsPtr > 0) { //guard for negative indexes
    _elementCounts[_elementCountsPtr++] = _currentRemainingElements;
}

doesn't work quite right. However, just adding the

if (_elementCounts.length == _elementCountsPtr) { // initially, as well as if full    
    _elementCounts = Arrays.copyOf(_elementCounts, _elementCounts.length+10);
}

guard to the methods I mentioned does the trick.

@cowtowncoder
Copy link
Member

Thank you for reporting this.

The usual version check: is this against 2.8.7 (or 2.9.0.pr1), or some older version?

@mlmitch
Copy link
Author

mlmitch commented Mar 16, 2017

Running with 2.8.6. The snippets of code I copied are from the github repository though (so 2.8.7, or 2.9.0?).

@cowtowncoder
Copy link
Member

Ok, this must be via contribution, and for 2.8. Obvious oversight, not sure how I did not catch it... especially since line 556 has proper handling (second one you suggested). I think it's ok not to check for negative indices given that code handles matching of start/end via states, although if it can become problematic it can definitely be added: these calls are not that numerous.

@mlmitch
Copy link
Author

mlmitch commented Mar 20, 2017

Thanks for fixing. You've been a real pro about the tickets I've submitted.

@cowtowncoder
Copy link
Member

Thank you for submitting them! Should go without saying that these make my work much easier, when there's full info on what is going wrong. And encoding/decoding failures are critical to fix as there's usually no work around.

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

No branches or pull requests

2 participants