Bug 66575 - Use of ByteBuffer.array() not guarded by hasArray()
Summary: Use of ByteBuffer.array() not guarded by hasArray()
Alias: None
Product: Tomcat 10
Classification: Unclassified
Component: WebSocket (show other bugs)
Version: unspecified
Hardware: PC Mac OS X 10.1
: P2 normal (vote)
Target Milestone: ------
Assignee: Tomcat Developers Mailing List
Depends on:
Reported: 2023-04-20 08:23 UTC by Simon Baslé
Modified: 2023-04-21 08:00 UTC (History)
0 users


Note You need to log in before you can comment on or make changes to this bug.
Description Simon Baslé 2023-04-20 08:23:24 UTC
This is just an example, and I'm pretty sure this is also the case in other classes, but in `org.apache.tomcat.websocket.PerMessageDeflate.java` a `ByteBuffer` is turned into a `byte[]` in order to deflate the payload, using the
`array()` method (see source: https://github.com/apache/tomcat/blob/main/java/org/apache/tomcat/websocket/PerMessageDeflate.java#L332)

The trouble is that not all `ByteBuffers` are backed by an `array()` or can return the `array()`: 
 - direct buffers are not backed by a `byte[]` at all
 - read-only buffers refuse to return their backing array (because it could be used to modify the contents)

This is well documented in the `ByteBuffer` javadoc which says that such accesses should be guarded by `hasArray()`.

The alternative (and portable) way of accessing the bytes for a read is to use one of the various `get(...)` methods, reading the bytes into a `byte[]`.
Note that this implies a copy.

Perhaps there should be a utility method in place for reading `byte[]` out of a `ByteBuffer` in Tomcat, using `array()` when possible (0-copy) and a variant of `get` otherwise?
Comment 1 Remy Maucherat 2023-04-20 09:03:38 UTC
Are you filing this issue as a general statement, or is this based on actual use ? Using direct ByteBuffers in user APIs likes this absolutely makes no sense at all (obviously).
Comment 2 Simon Baslé 2023-04-20 11:57:44 UTC
Sorry if I came across as a little blunt or if it sounded like I was reporting something just for the sake of perceived correctness. I should have focused on the fact and let you decide the way forward accordingly, I really just wanted to be thorough and helpful.

Also, please disregard my comment about unguarded `array()` calls in other places, I expect that in most cases the buffers are created by Tomcat so code can assume that these are heap buffers (not direct, not read-only).

But this is indeed based on actual use, according to a report from a Spring Framework user.

Spring sends a read-only `ByteBuffer` to a WebSocket endpoint via `RemoteEndpoint.Async#sendBinary`. The user reports that with Tomcat this leads to a `ReadOnlyBufferException` from the `PerMessageDeflate`.

From what I can see, the `ByteBuffer` passed to the endpoint is passed along to `Transformation`s which in this case includes the `PerMessageDeflate`. It ends up being used as the _input_ to the `Deflater`.
Comment 3 Mark Koszykowski 2023-04-20 12:56:05 UTC
In the specific example of the `PerMessageDeflate`, the issue of the ReadOnlyException can seemingly be resolved by instead using the `setInput​(ByteBuffer input)` method on the uncompressedPayload in  `sendMessagePart(List<MessagePart> uncompressedParts)`.

This method appears to account for different ByteBuffer access scenarios like this, and in my testing was able to resolve issues reported in the Spring Framework.
Comment 4 Christopher Schultz 2023-04-20 20:39:47 UTC
Can you submit a PR with a unit test using e.g. a read-only ByteBuffer and your proposed change?
Comment 5 Remy Maucherat 2023-04-20 21:49:08 UTC
(In reply to Christopher Schultz from comment #4)
> Can you submit a PR with a unit test using e.g. a read-only ByteBuffer and
> your proposed change?

I think I'll do it.
Comment 6 Remy Maucherat 2023-04-21 08:00:14 UTC
I shouldn't have asked for an explanation, user code can use direct or read only BB if it wants to, so it has to be supported.
The fix will be in 11.0.0-M6, 10.1.9, 9.0.75, 8.5.89.