search

FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
310 stars 133 forks source link

add tests for large nums #346

Closed pjfanning closed 1 year ago

pjfanning commented 1 year ago

These tests succeed despite the the large nums. This is because the numbers are represented in binary form and the text length checks do not get invoked. The question is whether we want to check the num lengths when we parse the binary nums.

cowtowncoder commented 1 year ago

Ok, so.... I think that.

  1. We DO want to maintain number length limits, but
  2. Limits should be imposed on binary encoded numbers (which will effectively allow bigger numbers since binary encoding is more efficient), NOT on String representation (unless there are specific cases where String representation must be caught)
  3. We could consider different default length limits for text-vs-binary. But I think we can start by using the same, worrying later if need be.

Given this, I think length limits need to be caught at decoding time, not when converting. So tests are probably fine, but I think implementation needs to be different.

pjfanning commented 1 year ago

@cowtowncoder I added some checks where the binary encoded numbers are decoded. I've updated the tests to match.

cowtowncoder commented 1 year ago

Thank you @pjfanning !