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 <mshep...@openjdk.org> wrote:
>
> On Thu, 13 Mar 2025 13:42:40 GMT, Volkan Yazici <vyaz...@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
