On 08/09/17 13:23, Konstantin Kolinko wrote:
> 2017-09-08 12:06 GMT+03:00  <ma...@apache.org>:
>> Author: markt
>> Date: Fri Sep  8 09:06:47 2017
>> New Revision: 1807693
>>
>> URL: http://svn.apache.org/viewvc?rev=1807693&view=rev
>> Log:
>> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=61491
>> When using the permessage-deflate extension, correctly handle the sending of 
>> empty messages after non-empty messages to avoid the IllegalArgumentException
>>
>> Added:
>>     
>> tomcat/tc7.0.x/trunk/test/org/apache/tomcat/websocket/TestPerMessageDeflate.java
>>       - copied, changed from r1807689, 
>> tomcat/tc8.5.x/trunk/test/org/apache/tomcat/websocket/TestPerMessageDeflate.java
>> Modified:
>>     tomcat/tc7.0.x/trunk/   (props changed)
>>     
>> tomcat/tc7.0.x/trunk/java/org/apache/tomcat/websocket/PerMessageDeflate.java
>>     tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml
> 
> 
> 1. There are local variables for uncompressedPart.getPayload()  and
> uncompressedPart.isFin()
> several lines below in this method. They can be moved up and used
> here. Though it does not matter much as those methods in MessagePart
> class are simple getters.
> 
>> ByteBuffer uncompressedPayload = uncompressedPart.getPayload();
> 
>> boolean fin = uncompressedPart.isFin();
> 
> 
> 2.  If sendMessagePart() is called with an empty part and this is not
> the last part,
> does the call have to be delivered to client, like some king of flush
> or ping? Or it can be ignored?

It can be ignored.

> If it is not ignored, and compression of (empty part) is not empty
>  (I may be wrong, but I expect it to contains some code to initialize
> compression table, as gz archive of an empty file is not empty),
> then I wonder whether the following works: sending two empty parts:
> {empty part, empty part & fin=true}

permessage-deflate does not include the header information in the
payload since that is negotiated already.

> From the code I think that the output will be {compressed part},
> followed by {passed-through empty "fin" part}? I think it will not be
> received correctly.

The first (non-fin) empty part is ignored.
The second (fin) empty part is sent uncompressed (since uncompressed
empty is smaller than compressed empty).

> 3. Javadoc for Deflater shows that end of input is signaled by calling
> finish();. I do not see such call here.

Correct. That would prevent the use of the compression context for the
next message.

Thanks for looking at this.

Mark

> 
> https://docs.oracle.com/javase/8/docs/api/java/util/zip/Deflater.html
> 
> Best regards,
> Konstantin Kolinko
> 
> 
>> Modified: 
>> tomcat/tc7.0.x/trunk/java/org/apache/tomcat/websocket/PerMessageDeflate.java
>> URL: 
>> http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/tomcat/websocket/PerMessageDeflate.java?rev=1807693&r1=1807692&r2=1807693&view=diff
>> ==============================================================================
>> --- 
>> tomcat/tc7.0.x/trunk/java/org/apache/tomcat/websocket/PerMessageDeflate.java 
>> (original)
>> +++ 
>> tomcat/tc7.0.x/trunk/java/org/apache/tomcat/websocket/PerMessageDeflate.java 
>> Fri Sep  8 09:06:47 2017
>> @@ -315,16 +315,20 @@ public class PerMessageDeflate implement
>>      public List<MessagePart> sendMessagePart(List<MessagePart> 
>> uncompressedParts) {
>>          List<MessagePart> allCompressedParts = new ArrayList<MessagePart>();
>>
>> +        // Flag to track if a message is completely empty
>> +        boolean emptyMessage = true;
>> +
>>          for (MessagePart uncompressedPart : uncompressedParts) {
>>              byte opCode = uncompressedPart.getOpCode();
>> +            boolean emptyPart = uncompressedPart.getPayload().limit() == 0;
>> +            emptyMessage = emptyMessage && emptyPart;
>>              if (Util.isControl(opCode)) {
>>                  // Control messages can appear in the middle of other 
>> messages
>>                  // and must not be compressed. Pass it straight through
>>                  allCompressedParts.add(uncompressedPart);
>> -            } else if (uncompressedPart.getPayload().limit() == 0 && 
>> uncompressedPart.isFin() &&
>> -                    deflater.getBytesRead() == 0) {
>> -                // Zero length messages can't be compressed so pass them
>> -                // straight through.
>> +            } else if (emptyMessage && uncompressedPart.isFin()) {
>> +                // Zero length messages can't be compressed so pass the
>> +                // final (empty) part straight through.
>>                  allCompressedParts.add(uncompressedPart);
>>              } else {
>>                  List<MessagePart> compressedParts = new 
>> ArrayList<MessagePart>();
>>
>> Copied: 
>> tomcat/tc7.0.x/trunk/test/org/apache/tomcat/websocket/TestPerMessageDeflate.java
>>  (from r1807689, 
>> tomcat/tc8.5.x/trunk/test/org/apache/tomcat/websocket/TestPerMessageDeflate.java)
>> URL: 
>> http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/test/org/apache/tomcat/websocket/TestPerMessageDeflate.java?p2=tomcat/tc7.0.x/trunk/test/org/apache/tomcat/websocket/TestPerMessageDeflate.java&p1=tomcat/tc8.5.x/trunk/test/org/apache/tomcat/websocket/TestPerMessageDeflate.java&r1=1807689&r2=1807693&rev=1807693&view=diff
>> ==============================================================================
>> --- 
>> tomcat/tc8.5.x/trunk/test/org/apache/tomcat/websocket/TestPerMessageDeflate.java
>>  (original)
>> +++ 
>> tomcat/tc7.0.x/trunk/test/org/apache/tomcat/websocket/TestPerMessageDeflate.java
>>  Fri Sep  8 09:06:47 2017
>> @@ -18,7 +18,6 @@ package org.apache.tomcat.websocket;
>>
>>  import java.io.IOException;
>>  import java.nio.ByteBuffer;
>> -import java.nio.charset.StandardCharsets;
>>  import java.util.ArrayList;
>>  import java.util.Collections;
>>  import java.util.List;
>> @@ -38,23 +37,23 @@ public class TestPerMessageDeflate {
>>
>>          // Set up the extension using defaults
>>          List<Parameter> parameters = Collections.emptyList();
>> -        List<List<Parameter>> preferences = new ArrayList<>();
>> +        List<List<Parameter>> preferences = new 
>> ArrayList<List<Parameter>>();
>>          preferences.add(parameters);
>>
>>          PerMessageDeflate perMessageDeflate = 
>> PerMessageDeflate.negotiate(preferences, true);
>>          perMessageDeflate.setNext(new TesterTransformation());
>>
>>          ByteBuffer bb1 = 
>> ByteBuffer.wrap("A".getBytes(StandardCharsets.UTF_8));
>> -        MessagePart mp1 = new MessagePart(true, 0, Constants.OPCODE_TEXT, 
>> bb1, null, null, -1);
>> +        MessagePart mp1 = new MessagePart(true, 0, Constants.OPCODE_TEXT, 
>> bb1, null, null);
>>
>> -        List<MessagePart> uncompressedParts1 = new ArrayList<>();
>> +        List<MessagePart> uncompressedParts1 = new ArrayList<MessagePart>();
>>          uncompressedParts1.add(mp1);
>>          perMessageDeflate.sendMessagePart(uncompressedParts1);
>>
>>          ByteBuffer bb2 = 
>> ByteBuffer.wrap("".getBytes(StandardCharsets.UTF_8));
>> -        MessagePart mp2 = new MessagePart(true, 0, Constants.OPCODE_TEXT, 
>> bb2, null, null, -1);
>> +        MessagePart mp2 = new MessagePart(true, 0, Constants.OPCODE_TEXT, 
>> bb2, null, null);
>>
>> -        List<MessagePart> uncompressedParts2 = new ArrayList<>();
>> +        List<MessagePart> uncompressedParts2 = new ArrayList<MessagePart>();
>>          uncompressedParts2.add(mp2);
>>          perMessageDeflate.sendMessagePart(uncompressedParts2);
>>      }
>>
>> Modified: tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml
>> URL: 
>> http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml?rev=1807693&r1=1807692&r2=1807693&view=diff
>> ==============================================================================
>> --- tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml (original)
>> +++ tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml Fri Sep  8 09:06:47 2017
>> @@ -100,6 +100,16 @@
>>        </fix>
>>      </changelog>
>>    </subsection>
>> +  <subsection name="WebSocket">
>> +    <changelog>
>> +      <fix>
>> +        <bug>61491</bug>: When using the <code>permessage-deflate</code>
>> +        extension, correctly handle the sending of empty messages after
>> +        non-empty messages to avoid the 
>> <code>IllegalArgumentException</code>.
>> +        (markt)
>> +      </fix>
>> +    </changelog>
>> +  </subsection>
>>    <subsection name="Tribes">
>>      <changelog>
>>        <fix>
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
>> For additional commands, e-mail: dev-h...@tomcat.apache.org
>>
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to