Christopher Tubbs created THRIFT-6080:
-----------------------------------------
Summary: Java binary types generate getters that incorrectly make
redundant array copies after resizing the array from the ByteBuffer source
Key: THRIFT-6080
URL: https://issues.apache.org/jira/browse/THRIFT-6080
Project: Thrift
Issue Type: Bug
Reporter: Christopher Tubbs
In the Java generator, for binary types, two getters are created, one that
looks like:
byte[] getFieldname();
And one that looks like:
ByteBuffer bufferForFieldname();
The first method seems to take the underlying bytes for the field, truncate it
to size, making a copy if needed, using TBaseHelper.rightSize(ByteBuffer).
Then, it makes an additional copy by calling the public API
setFieldname(ByteBuffer), which internally uses
TBaseHelper.copyBinary(ByteBuffer). All of this appears to be an attempt to
create a protective copy on read of the underlying byte array. However, what it
actually returns is the actual underlying array of the currently set
ByteBuffer. The next time the method is called, it will make another copy of
the ByteBuffer that is wrapping the array that was leaked outside the method
the last time it was called. This is not correct behavior if it is trying to
make a protective copy on read, and it is not correct behavior if it is trying
to merely truncate once and always return that same byte array. Neither
possible behavior is correct, so it makes no sense to keep making copies on
each read.
The second method appears to be designed to provide direct access to the
underlying ByteBuffer, presumably for efficiency if a copy of the byte array is
not needed. However, it also makes a protective copy of the bytes and creates a
new ByteBuffer object every time, by calling TBaseHelper.copyBinary(ByteBuffer)
each time. It appears that this was previously identified as a problem and a
flag was added (unsafe_binaries) to the Java generator to give direct access to
the underlying ByteBuffer without making a protective copy. However, the
problem with that flag is that it removes protective copies everywhere,
including in the constructor and in the setters, rather than just give direct
access to the underlying ByteBuffer. So, that flag makes it harder for calling
code to know when it needs to make its own protective copy and when it doesn't.
This was identified as a problem in
[https://github.com/apache/accumulo/pull/6455] ; after a few failed attempts to
work around the problems, we chose to avoid the expensive bufferForFieldname()
methods and the unsafe use of unsafe_binaries and patch the generated code at
build time in [https://github.com/apache/accumulo/pull/6459] to avoid the
problems with unnecessary copies of the array in the getter that returns an
array, by setting the internal field to the output of the
TBaseHelper.rightSize(ByteBuffer) method instead of setting it via the
setFieldname(ByteBuffer) method that makes the unnecessary copy. This still
gives callers access to the shared byte array instead of making a protective
copy of it each read, though. However, since it was already leaking the byte
array, and we are not modifying it, we found this acceptable behavior.
Ideally, the getter should be modified as we did in
[https://github.com/apache/accumulo/pull/6459,] but the array that is returned
should be returned with .clone() so it doesn't leak the original array after
resizing. This will potentially result in two copies the first time, and only
one copy each subsequent time. The alternative is to never modify the
underlying ByteBuffer, and just always make a copy each time, and never call
setFieldname(ByteBuffer). I think the unsafe_binaries flag is somewhat
worthless at this point... I think the entire purpose of having that method is
to get access to the underlying ByteBuffer directly. It makes no sense to be
generated at all, if it's going to make a protective copy. If it were going to
do that, it should just get the protective copy from the standard getter (once
fixed properly) and users can wrap it themselves with ByteBuffer.wrap(byte[])
if they really want to. The bufferFor methods should not be created at all by
default, unless requested to generate it using a new flag to be added for that
purpose. And when it is generated, it will always be generated without the
protective copy, because that's its entire reason for existing. Its behavior
should not be affected by the 'unsafe_binaries' flag at all. That flag (which I
recommend nobody ever use) should only affect the constructor and setters and
the getter that returns an array.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
