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

[Robert Engels]

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