Honor length in Tuple.fromBytes if provided#363
Conversation
f2e7491 to
e537808
Compare
| int pos = start; | ||
| int end = start + length; | ||
| while(pos < bytes.length) { | ||
| while(pos < end) { |
There was a problem hiding this comment.
What happens if end is past the end of the bytes array?
There was a problem hiding this comment.
In that case, you will get an ArrayIndexOutOfBoundsException on line 351 of TupleUtil when it tries do decode the value out of range:
Arguably, the correct thing is for this method (unpack) to do bounds checking early and then exit right then. I could add that, if we want.
There was a problem hiding this comment.
What does the rest of the Tuple layer do? If it behaves similarly to this, we can leave it and address it later as a whole if we want.
There was a problem hiding this comment.
I believe it generally does bounds checking at the last possible moment (i.e., when it gets the ArrayIndexOutOfBoundsException. That leads to another problem, which is that the decode logic for, say, floats will not necessarily do the right thing. For example:
Tuple.fromBytes(Tuple.from(3.14f).pack(), 0, 3)will return (3.14f) rather than throwing an error (because it doesn't check that the item is entirely located within the correct range). I think all of the fixed length types are like that, but the variable length things will stop before hitting their terminator.
There was a problem hiding this comment.
Ok, as we discussed a bit offline, I think we'll go ahead and merge this. If we want, we can raise a new issue to address the concern you just raised.
In Tuple unpack, we were looping until the end of the array rather than to the end of where the user specified. This fixes that. It now passes the little quick test I added within Tuple.java, but testing this more thoroughly would probably require more work in other places.
It also looks like the TLS related network options that were just added to fdb.options weren't added to the generated.go file, so I fixed that (in a separate commit).
This closes #362.