<html><body><div dir="ltr">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.</div><div dir="ltr"><br></div><div dir="ltr">Gruss</div><div dir="ltr">Bernd</div><div id="ms-outlook-mobile-body-separator-line" dir="ltr"></div><div id="ms-outlook-mobile-signature"><div dir="ltr">--</div><div dir="ltr">http://bernd.eckenfels.net</div></div><div> </div><hr style="display: inline-block; width: 98%;"><div id="divRplyFwdMsg" dir="ltr"><span style="font-family: Calibri, sans-serif;"><b>Von:</b> net-dev <net-dev-retn@openjdk.org> im Auftrag von Robert Engels <rengels@ix.netcom.com><br><b>Gesendet:</b> Freitag, März 14, 2025 4:01 AM<br><b>An:</b> Mark Sheppard <msheppar@openjdk.org><br><b>Cc:</b> net-dev@openjdk.org <net-dev@openjdk.org><br><b>Betreff:</b> Re: RFR: 8351339: WebSocket::sendBinary assume that user supplied buffers are BIG_ENDIAN [v2]</span><div style="font-family: Calibri, sans-serif;"> </div></div>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. 

<br>

<br>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. 

<br>

<br>> On Mar 13, 2025, at 9:27 PM, Mark Sheppard <msheppar@openjdk.org> wrote:

<br>> 

<br>> On Thu, 13 Mar 2025 13:42:40 GMT, Volkan Yazici <vyazici@openjdk.org> wrote:

<br>> 

>>> Fixes endian handling `jdk.internal.net.http.websocket.Frame.Masker`.

<br>>>> 

<br>>>> ### Implementation notes

<br>>>> 

>>> 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`.

<br>>> 

<br>>> Volkan Yazici has updated the pull request incrementally with one additional commit since the last revision:

<br>>> 

<br>>>  Fix copyright years

<br>> 

<br>> Expanding the paint job on the bikeshed for Masker

<br>> 

<br>> I’d proffer some alternative names to the current on offer and a possible restructuring of Masker providing a set of static  utility

<br>> 

> I haven’t looked at any use cases for Masker and the use relationship between Frame and Masker.

<br>> 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).

<br>> 

<br>> Masker purpose is to hold state

<br>> 

<br>> private final int[] maskBytes = new int[4];

<br>> private int offset;

<br>> private long maskLongBe;  // maybe rename beLongMask

<br>> private long maskLongLe;  // maybe rename leLongMask

<br>> 

> Which in turn could be encapsulated in a Mask class.  A Mask is instantiated in the Masker.createMask(int mask) method (formerly reset).

<br>> 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

<br>> 

> 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 ?

<br>> 

<br>> Looking at the proposed methods, then

<br>> 

<br>> Alternative names to the static “mask” method  are    copyWithMask to transferWithMask

<br>> 

<br>> mask  —> applyMask      // the non static mask

<br>> initGallopingMasking —> initLongMask

<br>> doGallopingMasking —> applyLongMask

<br>> doPlainMAsking  —> endApplyMask

<br>> 

> Using a Mask abstraction to contain the state of the mask processing, then reset becomes a factory method createMask() returning a Mask object.

<br>> 

<br>> And this instance of Mask is passed in the processing call chain

<br>> 

<br>>          class Mask  {

<br>>                public  Mask ( int[] maskBytes, int offset,long beLongMask,long leLongMask) {

<br>>            . . .

<br>>          }

<br>> 

<br>>       }

<br>> 

<br>>      static void mask(ByteBuffer src, ByteBuffer dst, int mask) {

<br>>            if (src.remaining() > dst.remaining()) {

<br>>                throw new IllegalArgumentException(dump(src, dst));

<br>>            }

<br>>            Mask theMask = createMask(mask);

<br>>            applyMask(src, dot, theMask);

<br>>         }

<br>> 

<br>>        /*

>         * Clears this instance's state and sets the mask.

<br>>         *

>         * The behaviour is as if the mask was set on a newly created instance.

<br>>         */

<br>>        public Mask createMask(int mask) {

<br>>            ByteBuffer acc = ByteBuffer.allocate(8).putInt(mask).putInt(mask).flip();

<br>>            final int[] maskBytes = new int[4];

<br>>            for (int i = 0; i < maskBytes.length; i++) {

<br>>                maskBytes[i] = acc.get(i);

<br>>            }

<br>>            offset = 0;

<br>>            beLongMask = acc.getLong(0);

<br>>            leLongMask = Long.reverseBytes(maskLongBe);

<br>>            return new Mask(maskBytes, offset, beLongMask, leLongMask);

<br>>        }

<br>> 

<br>>        /*

<br>>         * Reads as many remaining bytes as possible from the given input

<br>>         * buffer, masks them with the previously set mask and writes the

>         * resulting bytes to the given output buffer.

<br>>         *

<br>>         * 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.

<br>>         */

<br>>        public void applyMask(ByteBuffer src, ByteBuffer dst, theMask) {

<br>>            if (src.order() == dst.order()) {

<br>>                initLongMask(src, dst, theMask);

<br>>                applyLongMask(src, dst, theMask);

<br>>            }

<br>>            endApplyMask(src, dst, theMask);

<br>>        }

<br>> 

<br>>        /**

>         * Positions the {@link #offset} at 0, which is needed for galloping, by masking necessary amount of bytes.

<br>>         */

<br>>        private static void initLongMask(ByteBuffer src, ByteBuffer dst, Mask aMask) {

<br>>         . . .

<br>>        }

<br>> 

<br>> 

<br>>       private static void applyLongMask(ByteBuffer src, ByteBuffer dst, Mask aMask) {

<br>>       . . .

<br>>       }

<br>> 

<br>>       private void endApplyMask(ByteBuffer src, ByteBuffer dst, Mask aMask) {

<br>>       . . .

<br>>       }

<br>> 

<br>> -------------

<br>> 

<br>> PR Comment: https://git.openjdk.org/jdk/pull/24033#issuecomment-2723180720

<br></body></html>