RFR: 8351339: WebSocket::sendBinary assume that user supplied buffers are BIG_ENDIAN [v2]

Fri Mar 14 04:13:31 UTC 2025

Yes. More garbage engineering. The effective chance of this is 0 so setting mask to 1 isn’t really doing anything. Sometimes I’m really embarrassed by my colleagues - the fact that this made it into the spec is just insane. 

> On Mar 13, 2025, at 10:24 PM, Bernd <ecki at zusammenkunft.net> wrote:
> 
> 
> The implementation has to read them because websockets use an odd xor masking, and the implementation has optimized this with longs, which have this endian problem, (yes it’s a bug). Maybe some vector api can replace the handmade procedure in the future without caring for the declared endianess.
> 
> Gruss
> Bernd
> --
> http://bernd.eckenfels.net
>  
> Von: net-dev <net-dev-retn at openjdk.org> im Auftrag von Robert Engels <rengels at ix.netcom.com>
> Gesendet: Freitag, März 14, 2025 4:01 AM
> An: Mark Sheppard <msheppar at openjdk.org>
> Cc: net-dev at openjdk.org <net-dev at openjdk.org>
> Betreff: Re: RFR: 8351339: WebSocket::sendBinary assume that user supplied buffers are BIG_ENDIAN [v2]
>  
> No. It is a simple bug. The payload is defined to be a byte stream. It should never be manipulated. The implementation MUST send the bytes exactly as the api caller provided.
> 
> But I think there is something else at play - the implementation should not be reading the bytes at all - which is the only time byte order would matter - something larger than a byte.
> 
> > On Mar 13, 2025, at 9:27 PM, Mark Sheppard <msheppar at openjdk.org> wrote:
> >
> > On Thu, 13 Mar 2025 13:42:40 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:
> >
> >>> Fixes endian handling `jdk.internal.net.http.websocket.Frame.Masker`.
> >>>
> >>> ### Implementation notes
> >>>
> >>> I deleted the `Frame` clone in tests, and rewired the test code depending on it to the actual `Frame`. To enable this, I relaxed the visibility of the actual `Frame`. I guess the `Frame` clone was introduced to have strict visibility in the actual `Frame`. Though this is not needed since the actual `Frame` is in an internal package. Plus, the fact that bug is in the `Frame` class hints in the direction that there should be one `Frame`.
> >>
> >> Volkan Yazici has updated the pull request incrementally with one additional commit since the last revision:
> >>
> >> Fix copyright years
> >
> > Expanding the paint job on the bikeshed for Masker
> >
> > I’d proffer some alternative names to the current on offer and a possible restructuring of Masker providing a set of static utility
> >
> > I haven’t looked at any use cases for Masker and the use relationship between Frame and Masker.
> > As such, working under the assumption that by not having a factory method or an explicit constructor, its
> > instantiation is purely meant to be in the context of the mask operation (formerly transferMasking).
> >
> > Masker purpose is to hold state
> >
> > private final int[] maskBytes = new int[4];
> > private int offset;
> > private long maskLongBe; // maybe rename beLongMask
> > private long maskLongLe; // maybe rename leLongMask
> >
> > Which in turn could be encapsulated in a Mask class. A Mask is instantiated in the Masker.createMask(int mask) method (formerly reset).
> > And this Mask instance is passed as a parameter to various masking operations in the mask call flow. Masker’s method then become become static methods
> >
> > Reset is not really a reset but more an init (substituting for a constructor) and its returning Masker is purely to facilitate, the compound invocation chain in mask.
> > Masker objects have a transient existence, are there any GC implication with the current instantiation use case?
> > It is possible to collapse reset into a Masker constructor ?
> >
> > Looking at the proposed methods, then
> >
> > Alternative names to the static “mask” method are copyWithMask to transferWithMask
> >
> > mask —> applyMask // the non static mask
> > initGallopingMasking —> initLongMask
> > doGallopingMasking —> applyLongMask
> > doPlainMAsking —> endApplyMask
> >
> > Using a Mask abstraction to contain the state of the mask processing, then reset becomes a factory method createMask() returning a Mask object.
> >
> > And this instance of Mask is passed in the processing call chain
> >
> > class Mask {
> > public Mask ( int[] maskBytes, int offset,long beLongMask,long leLongMask) {
> > . . .
> > }
> >
> > }
> >
> > static void mask(ByteBuffer src, ByteBuffer dst, int mask) {
> > if (src.remaining() > dst.remaining()) {
> > throw new IllegalArgumentException(dump(src, dst));
> > }
> > Mask theMask = createMask(mask);
> > applyMask(src, dot, theMask);
> > }
> >
> > /*
> > * Clears this instance's state and sets the mask.
> > *
> > * The behaviour is as if the mask was set on a newly created instance.
> > */
> > public Mask createMask(int mask) {
> > ByteBuffer acc = ByteBuffer.allocate(8).putInt(mask).putInt(mask).flip();
> > final int[] maskBytes = new int[4];
> > for (int i = 0; i < maskBytes.length; i++) {
> > maskBytes[i] = acc.get(i);
> > }
> > offset = 0;
> > beLongMask = acc.getLong(0);
> > leLongMask = Long.reverseBytes(maskLongBe);
> > return new Mask(maskBytes, offset, beLongMask, leLongMask);
> > }
> >
> > /*
> > * Reads as many remaining bytes as possible from the given input
> > * buffer, masks them with the previously set mask and writes the
> > * resulting bytes to the given output buffer.
> > *
> > * The source and the destination buffers may be the same instance. If
> > * the mask hasn't been previously set it is assumed to be 0.
> > */
> > public void applyMask(ByteBuffer src, ByteBuffer dst, theMask) {
> > if (src.order() == dst.order()) {
> > initLongMask(src, dst, theMask);
> > applyLongMask(src, dst, theMask);
> > }
> > endApplyMask(src, dst, theMask);
> > }
> >
> > /**
> > * Positions the {@link #offset} at 0, which is needed for galloping, by masking necessary amount of bytes.
> > */
> > private static void initLongMask(ByteBuffer src, ByteBuffer dst, Mask aMask) {
> > . . .
> > }
> >
> >
> > private static void applyLongMask(ByteBuffer src, ByteBuffer dst, Mask aMask) {
> > . . .
> > }
> >
> > private void endApplyMask(ByteBuffer src, ByteBuffer dst, Mask aMask) {
> > . . .
> > }
> >
> > -------------
> >
> > PR Comment: https://git.openjdk.org/jdk/pull/24033#issuecomment-2723180720
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/net-dev/attachments/20250313/201524f4/attachment.htm>