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

_inputPtr off-by-one in UTF8StreamJsonParser._parseNumber2() #516

Closed
gsson opened this issue Feb 13, 2019 · 5 comments
Closed

_inputPtr off-by-one in UTF8StreamJsonParser._parseNumber2() #516

gsson opened this issue Feb 13, 2019 · 5 comments
Milestone

Comments

@gsson
Copy link

gsson commented Feb 13, 2019

When an input contains space separated root level values, and the input is split such that _parseNumber2() is invoked, _inputPtr will become incremented once in the method

        // As per #105, need separating space between root values; check here
        if (_parsingContext.inRoot()) {
            _verifyRootSpace(_inputBuffer[_inputPtr++] & 0xFF);
        }

and once in _verifyRootSpace():

        // caller had pushed it back, before calling; reset
        ++_inputPtr;

causing the next token to lose its first character.

Related to #105.

Failing test case:

package test;

import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonParser;
import org.junit.Test;

import java.io.IOException;
import java.io.InputStream;
import java.util.stream.Stream;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class JacksonIssue {
    @Test
    public void failingRootLevelParsing() throws IOException {
        // InputStream that forces _parseNumber2 to be invoked.
        InputStream is = TestInputStream.fromStrings("1234", "5 true");

        JsonFactory jsonFactory = new JsonFactory();
        JsonParser parser = jsonFactory.createParser(is);

        // Works!
        assertEquals(12345, parser.nextIntValue(0));

        // Fails with com.fasterxml.jackson.core.JsonParseException: Unrecognized token 'rue': was expecting ('true', 'false' or 'null')
        assertTrue(parser.nextBooleanValue());

    }

    static class TestInputStream extends InputStream {
        private final byte[][] reads;
        private int currentRead;

        public static TestInputStream fromStrings(String ... strings) {
            byte[][] reads = Stream.of(strings)
                    .map(s -> s.getBytes(UTF_8))
                    .toArray(byte[][]::new);
            return new TestInputStream(reads);
        }

        public TestInputStream(byte[][] reads) {
            this.reads = reads;
            this.currentRead = 0;
        }

        @Override
        public int read() throws IOException {
            throw new UnsupportedOperationException();
        }

        @Override
        public int read(byte[] b, int off, int len) throws IOException {
            if (currentRead >= reads.length) {
                return -1;
            }
            byte[] bytes = reads[currentRead++];
            if (len < bytes.length) {
                throw new IllegalArgumentException();
            }
            System.arraycopy(bytes, 0, b, off, bytes.length);
            return bytes.length;
        }
    }
}
@cowtowncoder
Copy link
Member

Thank you for reporting this. I hope to verify this as soon as possible, added it to work-in-progress page.

@gsson
Copy link
Author

gsson commented Feb 13, 2019

I think the fix is simply to remove the post-increment of _inputPtr in this line of parseNumber2():

        if (_parsingContext.inRoot()) {
            _verifyRootSpace(_inputBuffer[_inputPtr++] & 0xFF);
        }

but there's a lot of index juggling going on, so I'm leaving that entirely up to you :)

@gsson
Copy link
Author

gsson commented Feb 13, 2019

(Note that this issue doesn't affect me in any way, just stumbled upon it while reading the code looking for something else)

@cowtowncoder
Copy link
Member

Thank you for reporting it. Rarely encountered edge cases are pretty nasty exactly as they do not get reported as quickly as common ones. I am bit surprised I didn't notice this when I went over with last pass but better late than never. Regression test is essential here.

@cowtowncoder
Copy link
Member

@gsson I think you are absolutely right wrt removing it. Code is bit awkward there (in other loops there's no need to try to re-access it), but it's not the common case. Reader-based parser actually does not trip in this case, as code is structured bit differently.

cowtowncoder added a commit that referenced this issue Feb 14, 2019
@cowtowncoder cowtowncoder added this to the 2.9.9 milestone Feb 14, 2019
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