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 <e...@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-r...@openjdk.org> im Auftrag von Robert Engels > <reng...@ix.netcom.com> > Gesendet: Freitag, März 14, 2025 4:01 AM > An: Mark Sheppard <mshep...@openjdk.org> > Cc: net-dev@openjdk.org <net-dev@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 <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
