TooTallNate / Java-WebSocket

A barebones WebSocket client and server implementation written in 100% Java.
http://tootallnate.github.io/Java-WebSocket
MIT License
10.54k stars 2.58k forks source link

PerMessageDeflateExtension#setDeflater()/#setInflater() is overwritten in case of no_context_takeover #1400

Closed robert-s-ubi closed 2 weeks ago

robert-s-ubi commented 9 months ago

Describe the bug In the PerMessageDeflateExtension class, the setDeflater() and setInflater() methods set the internal deflater and inflater objects. However, in case of client|serverNoClientTakeover being true (the latter is the default), the set objects will be overwritten internally by their hardcoded defaults. As this happens during data exchange, there is no obvious hook by which the application could repeat the setDeflater() and setInflater() calls.

Relevant excerpts from PerMessageDeflateExtension.java:

private boolean serverNoContextTakeover = true; private boolean clientNoContextTakeover = false;

private Inflater inflater = new Inflater(true); private Deflater deflater = new Deflater(Deflater.DEFAULT_COMPRESSION, true);

public void setInflater(Inflater inflater) { this.inflater = inflater; }

public void setDeflater(Deflater deflater) { this.deflater = deflater; }

@Override public void decodeFrame(Framedata inputFrame) throws InvalidDataException { ... // If context takeover is disabled, inflater can be reset. if (clientNoContextTakeover) { inflater = new Inflater(true); } ...

@Override public void encodeFrame(Framedata inputFrame) { ... if (serverNoContextTakeover) { deflater.end(); deflater = new Deflater(Deflater.DEFAULT_COMPRESSION, true); } ...

Expected behavior The Inflater and Deflater objects set once should be retained throughout the lifetime of a class instance. I'm not sure why the current implementation creates new objects instead of calling inflater.reset() and deflater.reset(), respectively. If the reset calls achieve the same result as recreating the objects, it is a simple two-line fix. If, however, new object instances are indeed required, the API would have to be changed to pass builder objects instead of object instances.

robert-s-ubi commented 9 months ago

On further investigation, I found that actually none of the settings have any effect, because WebSocketImpl() calls copyInstance() on the PerMessageDeflateExtension instance, which creates a new object instance with the default settings.

Leading me to the conclusion that the second line in this excerpt of AutobahnServerTest also has no effect:

PerMessageDeflateExtension perMessageDeflateExtension = new PerMessageDeflateExtension();
**perMessageDeflateExtension.setThreshold(0);**
AutobahnServerTest test = new AutobahnServerTest(port, limit,
    new Draft_6455(perMessageDeflateExtension));

The threshold used will be 1024 despite this line. This is also what I observed when using the library in my code.

marci4 commented 1 month ago

This should be addressed with #1439