Should _ensureRoom in ProtobufGenerator.writeString()?

[marsqing]

With version 2.9.0.pr3, we encounter a ArrayIndexOutOfBoundsException

Caused by: java.lang.ArrayIndexOutOfBoundsException: 8000
         at com.fasterxml.jackson.dataformat.protobuf.ProtobufUtil.appendLengthLength(ProtobufUtil.java:73)
         at com.fasterxml.jackson.dataformat.protobuf.ProtobufGenerator._writeLengthPrefixed(ProtobufGenerator.java:1293)
         at com.fasterxml.jackson.dataformat.protobuf.ProtobufGenerator._encodeLongerString(ProtobufGenerator.java:1286)
         at com.fasterxml.jackson.dataformat.protobuf.ProtobufGenerator.writeString(ProtobufGenerator.java:729)
         at com.fasterxml.jackson.databind.ser.std.StringSerializer.serialize(StringSerializer.java:41)
         at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:727)
         at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:716)
         ... 104 more

Occurs once every few hours, so not easily reproduced. Shall we call _ensureRoom when _encodeLongerString?

[cowtowncoder]

Sounds and looks like a bug. Check for space should be done in _writeLengthPrefixed() I think (for both _writeTag() and appendLengthLength()). But it does get bit tricky for case(s) where there's buffer boundary.... probably should check to see if _currBuffer has minimum required to write both (10 bytes I think?); if not, switch to codepath that can flush/append buffer.

Reproduction would be great although I understand it is not trivial to write.

[marsqing]

Can be reproduced by the following code:

import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.dataformat.protobuf.ProtobufMapper;
import com.fasterxml.jackson.dataformat.protobuf.schema.ProtobufSchema;
import com.fasterxml.jackson.dataformat.protobuf.schemagen.ProtobufSchemaGenerator;
import java.io.ByteArrayOutputStream;
import org.junit.Test;

/**
 * Created by marsqing on 14/06/2017.
 */
public class StringTest {

  @Test
  public void stringTest() throws Exception {
    Pojo13 p = new Pojo13();
    // near 8000, so index out of bounds at 8000
    p.setValue1(makeString(7995));
    p.setValue2(makeString(7995));

    ByteArrayOutputStream bout = new ByteArrayOutputStream();
    ProtobufMapper mapper = new ProtobufMapper();
    mapper.writer(getSchema(mapper, p.getClass())).writeValue(bout, p);
  }

  private ProtobufSchema getSchema(ObjectMapper mapper, Class<?> clazz) throws Exception {
    ProtobufSchemaGenerator gen = new ProtobufSchemaGenerator();
    mapper.acceptJsonFormatVisitor(clazz, gen);
    return gen.getGeneratedSchema();
  }

  private String makeString(int len) {
    StringBuilder sb = new StringBuilder();
    for (int i = 0; i < len; i++) {
      sb.append("a");
    }
    return sb.toString();
  }

 public static class Pojo13 {
    private String value1;
    private String value2;

    public Pojo13() {
    }

    public String getValue1() {
      return value1;
    }

    public void setValue1(String value1) {
      this.value1 = value1;
    }

    public String getValue2() {
      return value2;
    }

    public void setValue2(String value2) {
      this.value2 = value2;
    }
  }

}

Caused by: java.lang.ArrayIndexOutOfBoundsException: 8000
    at com.fasterxml.jackson.dataformat.protobuf.ProtobufUtil.appendLengthLength(ProtobufUtil.java:76)
    at com.fasterxml.jackson.dataformat.protobuf.ProtobufGenerator._writeLengthPrefixed(ProtobufGenerator.java:1293)
    at com.fasterxml.jackson.dataformat.protobuf.ProtobufGenerator._encodeLongerString(ProtobufGenerator.java:1286)
    at com.fasterxml.jackson.dataformat.protobuf.ProtobufGenerator.writeString(ProtobufGenerator.java:729)
    at com.fasterxml.jackson.databind.ser.std.StringSerializer.serialize(StringSerializer.java:41)
    at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:727)
    at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:716)
    ... 29 more

[383124397]

so, I hope this problem could be fixed as soon as possible, thanks both of you : )

[cowtowncoder]

Yup, I'll try to get it fixed soon. Just need to re-familiarize with specific part -- unlike with many other jackson codecs, solution here is not to simplify flush contents (since due to nesting, not everything can actually be flushed)

[cowtowncoder]

Ah. Actually, just like title said, just call ensureRoom(). :-D

It'd be good to have a test for nested case too, but since this method is used elsewhere I think it's ok to rely on its working.

Unfortunately this went in 2.8 branch right after 2.8.9. On plus side it gets in 2.9.0.pr4 which I will release very soon. Wrt 2.8, I could release 2.8.9.1 micro-patch, since 2.8.10 will not be released any time soon. But I will have to release micro-patch of jackson-databind too.

[marsqing]

Great! Is it also necessary to call ensureRoom() in _writeEnum and maybe other functions?

[cowtowncoder]

@marsqing which lines? I tried to have a quick look to see if there are obvious emission, but may have missed something. I'll check out _writeEnum now.

[cowtowncoder]

Ok, no: _writeEnum is fine as it checks for boundary itself (either directly, or in _writeVInt). But thank you for asking, better safe than sorry.