parseJSON with originTracking enabled creates broken offset/length information if the input exceeds 1024 characters

[urbanfly]

Describe the bug

When certain JSON data is parsed into an ADT, it fails with an error about length should be positive while constructing the SourceLocation.

To Reproduce

Run the following test using Rascal v0.41.2 (bundled with the Rascal VS Code extension v0.130)

data OuterData = outer(str name, str \type, list[Nested] nested_abc);
data Nested = nested(str \type, map[str, value] property__1, map[value, value] property__2);

test bool failsOnCertainJSON() {
    str input = "{
                '  \"name\": \"TESTING Adding one more character to this data causes the test to fail\",
                '  \"type\": \"type_abcd\",
                '  \"nested_abc\": [
                '    {
                '      \"type\": \"line\",
                '      \"property__1\": { \"y\": { \"value\": 2, \"absolute\": false } },
                '      \"property__2\": {}
                '    },
                '    {
                '      \"type\": \"line\",
                '      \"property__1\": { \"y\": { \"value\": 2, \"absolute\": false } },
                '      \"property__2\": {}
                '    },
                '    {
                '      \"type\": \"line\",
                '      \"property__1\": { \"y\": { \"value\": 2, \"absolute\": false } },
                '      \"property__2\": {}
                '    },
                '    {
                '      \"type\": \"line\",
                '      \"property__1\": { \"y\": { \"value\": 2, \"absolute\": false } },
                '      \"property__2\": {}
                '    },
                '    {
                '      \"type\": \"line\",
                '      \"property__1\": { \"y\": { \"value\": 2, \"absolute\": false } },
                '      \"property__2\": {}
                '    },
                '    {
                '      \"type\": \"line\",
                '      \"property__1\": { \"y\": { \"value\": 2, \"absolute\": false } },
                '      \"property__2\": {}
                '    },
                '    {
                '      \"type\": \"line\",
                '      \"property__1\": { \"y\": { \"value\": 2, \"absolute\": false } },
                '      \"property__2\": {}
                '    }
                '  ]
                '}
                '";

    // Works on any (?) data
    parsedMap = parseJSON(#map[str, value], input);
    println("Parsed JSON (Before)");
    iprintln(parsedMap);

    // Works with the data as-is
    parsedVFD = parseJSON(#OuterData, input);
    println("Parsed VisualFormData (Before)");
    iprintln(parsedVFD);

    // Insert a character in the string to test the boundary condition
    input = replaceFirst(input, "TESTING", "TESTING_");

    // Still works (expected)
    parsedMap = parseJSON(#map[str, value], input);
    println("Parsed JSON (After)");
    iprintln(parsedMap);

    // Fails when the original data above is increased by even one character (e.g. the name, or changing a 2 to 20, or even adding a meaningless space anywhere)
    parsedVFD = parseJSON(#OuterData, input);
    println("Parsed VisualFormData (After)");
    iprintln(parsedVFD);

    return true;
}

Expected behavior

The JSON is parsed into the ADT, obviously :)

Stack traces

Java("IllegalArgumentException","length should be positive")
 at io.usethesource.vallang.impl.primitive.SourceLocationValues.newSourceLocation(|unknown:///SourceLocationValues.java|(0,0,<58,0>,<58,0>))
 at io.usethesource.vallang.impl.primitive.AbstractPrimitiveValueFactory.sourceLocation(|unknown:///AbstractPrimitiveValueFactory.java|(0,0,<211,0>,<211,0>))
 at io.usethesource.vallang.impl.fields.AbstractValueFactoryAdapter.sourceLocation(|unknown:///AbstractValueFactoryAdapter.java|(0,0,<177,0>,<177,0>))
 at org.rascalmpl.library.lang.json.internal.JsonValueReader$ExpectedTypeDispatcher.visitObjectAsAbstractData(|unknown:///JsonValueReader.java|(0,0,<693,0>,<693,0>))
 at org.rascalmpl.library.lang.json.internal.JsonValueReader$ExpectedTypeDispatcher.visitAbstractData(|unknown:///JsonValueReader.java|(0,0,<718,0>,<718,0>))
 at org.rascalmpl.library.lang.json.internal.JsonValueReader$ExpectedTypeDispatcher.visitAbstractData(|unknown:///JsonValueReader.java|(0,0,<76,0>,<76,0>))
 at io.usethesource.vallang.type.AbstractDataType.accept(|unknown:///AbstractDataType.java|(0,0,<418,0>,<418,0>))
 at org.rascalmpl.library.lang.json.internal.JsonValueReader$ExpectedTypeDispatcher.visitList(|unknown:///JsonValueReader.java|(0,0,<845,0>,<845,0>))
 at org.rascalmpl.library.lang.json.internal.JsonValueReader$ExpectedTypeDispatcher.visitList(|unknown:///JsonValueReader.java|(0,0,<76,0>,<76,0>))
 at io.usethesource.vallang.type.ListType.accept(|unknown:///ListType.java|(0,0,<240,0>,<240,0>))
 at org.rascalmpl.library.lang.json.internal.JsonValueReader$ExpectedTypeDispatcher.visitObjectAsAbstractData(|unknown:///JsonValueReader.java|(0,0,<637,0>,<637,0>))
 at org.rascalmpl.library.lang.json.internal.JsonValueReader$ExpectedTypeDispatcher.visitAbstractData(|unknown:///JsonValueReader.java|(0,0,<718,0>,<718,0>))
 at org.rascalmpl.library.lang.json.internal.JsonValueReader$ExpectedTypeDispatcher.visitAbstractData(|unknown:///JsonValueReader.java|(0,0,<76,0>,<76,0>))
 at io.usethesource.vallang.type.AbstractDataType.accept(|unknown:///AbstractDataType.java|(0,0,<418,0>,<418,0>))
 at org.rascalmpl.library.lang.json.internal.JsonValueReader.read(|unknown:///JsonValueReader.java|(0,0,<1028,0>,<1028,0>))
 at org.rascalmpl.library.lang.json.IO.parseJSON(|unknown:///IO.java|(0,0,<103,0>,<103,0>))
 at parseJSON(|std:///lang/json/IO.rsc|(7724,5,<138,27>,<138,32>))

Additional context

This test passes with Rascal v0.40.x (bundled with VS Code ext. v0.12.2)

[jurgenvinju]

Thanks for the clear report. Will look into this soon

[jurgenvinju]

• We bumped the gson dependency since the working version of rascal
• We use infernal fields of gson to uncover it's internal cursor on the file.
• This piece of code is something to look into:

private IValue visitObjectAsAbstractData(Type type) throws IOException {
            Set<Type> alternatives = null;

            in.beginObject();
            int startPos = getPos();
            int startLine = getLine();
            int startCol = getCol();

Note to self: what if gson decided to shift positions now on reading { with .beginObjectwhile it didn't do this before. That would (a) be a typical improvement which increases accuracy of error messages and (b) explain our erroneous behavior. It's a guess, but that's what I'll try to write a test for when I've a laptop again.

[jurgenvinju]

@urbanfly Robert, this bug must be rather subtle because I can't get the example to fail. I've been changing characters, adding spaces, changing 2's to 20's, and lots of other stuff, but I keep getting the ADT parsed with the right contents. Love a good puzzel :-)

Could you send me a file or string that certainly fails on your machine? Much appreciated!

Never mind; @DavyLandman suggested adding some Windows newlines, and boom.

[jurgenvinju]

• it fails only when there are \r characters in place, as by Windows newline encoding
• it does not matter if the JSON is embedded in a Rascal file with Windows characters in the template string, or if it is in a file with unix2dos applied on Mac. We get the same failure nicely every time now.
• debug stepping through the example: it's not a difference of encoding, but a difference of length of whitespace. With the \r characters in place the example is just big enough between the objects to cause a corner case where the interal pos parsing offset has been reset to 0 (while indexing into a buffer) between expecting and accepting the last Nested object. This can never happen on a single line. I can create an example now that fails on Unix as well.
• the BUFFER_SIZE = 1024 and the failing example is 1044 at this moment. This confirms there has been a buffer resize event during parsing.
• it appears although the vallang library did not complain too often, this bug is wider than the exception being thrown. Positions must have been off more often due to this bug.

[jurgenvinju]

Changed the title to reflect the latest diagnostics. Now pondering a solution that hopefully does not entail enlarging the streaming buffer to the file size ;-)

[urbanfly]

FYI: I did try using trackOrigins=false, but it still failed.

[jurgenvinju]

Thanks @urbanfly for checking that out. The code uses the origin information internally to be able to produce accurate error messages, so turning off the trackOrigins here doesn't avoid the fawlty computations. In that sense trackOrigins is a misnomer, but we kept it for consistency with other parsing functions (for XML, YAML, etc).

I have fix here #2638 and working on the code quality part of that now. At least this is my current output. As you can see the final nested object starts just before the 1024 boundary, and it has length 124 making it end after gson has reset its internal buffer and pos field.

The only way to break this now is having a comment line longer than 1024 characters on a single line. I don't know how to fix that yet. Or maybe this is it.

outer(
  "TESTING  Adding one more character to this data causes the test to fail",
  "type_abcd",
  [
    nested(
      "line",
      ("y":"object"(
          value=2,
          absolute=false)),
      (),
      src=|unknown:///|(137,122,<5,5>,<9,6>)),
    nested(
      "line",
      ("y":"object"(
          value=2,
          absolute=false)),
      (),
      src=|unknown:///|(266,122,<10,5>,<14,6>)),
    nested(
      "line",
      ("y":"object"(
          value=2,
          absolute=false)),
      (),
      src=|unknown:///|(395,122,<15,5>,<19,6>)),
    nested(
      "line",
      ("y":"object"(
          value=2,
          absolute=false)),
      (),
      src=|unknown:///|(524,122,<20,5>,<24,6>)),
    nested(
      "line",
      ("y":"object"(
          value=2,
          absolute=false)),
      (),
      src=|unknown:///|(653,122,<25,5>,<29,6>)),
    nested(
      "line",
      ("y":"object"(
          value=2,
          absolute=false)),
      (),
      src=|unknown:///|(782,124,<30,5>,<34,8>)),
    nested(
      "line",
      ("y":"object"(
          value=2,
          absolute=false)),
      (),
      src=|unknown:///|(913,124,<35,5>,<39,6>))
  ],
  src=|unknown:///|(1,1062,<1,1>,<41,2>))