Mailing-List: contact dev-help@tomcat.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Tomcat Developers List" <dev@tomcat.apache.org>
MIME-Version: 1.0
In-Reply-To: <20170908090648.2F81B3A0051@svn01-us-west.apache.org>
References: <20170908090648.2F81B3A0051@svn01-us-west.apache.org>
From: Konstantin Kolinko <knst.kolinko@gmail.com>
Date: Fri, 8 Sep 2017 15:23:43 +0300
Message-ID: <CABzHfVkxV2W31m=giAMFq6yO7NpB8AdtM6rqMWXMhbu6LS-DwA@mail.gmail.com>
Subject: Re: svn commit: r1807693 - in /tomcat/tc7.0.x/trunk: ./
 java/org/apache/tomcat/websocket/PerMessageDeflate.java test/org/apache/tomcat/websocket/TestPerMessageDeflate.java
 webapps/docs/changelog.xml
To: Tomcat Developers List <dev@tomcat.apache.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
archived-at: Fri, 08 Sep 2017 12:23:51 -0000

2017-09-08 12:06 GMT+03:00  <markt@apache.org>:
> Author: markt
> Date: Fri Sep  8 09:06:47 2017
> New Revision: 1807693
>
> URL: http://svn.apache.org/viewvc?rev=3D1807693&view=3Drev
> Log:
> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=3D61491
> When using the permessage-deflate extension, correctly handle the sending=
 of empty messages after non-empty messages to avoid the IllegalArgumentExc=
eption
>
> Added:
>     tomcat/tc7.0.x/trunk/test/org/apache/tomcat/websocket/TestPerMessageD=
eflate.java
>       - copied, changed from r1807689, tomcat/tc8.5.x/trunk/test/org/apac=
he/tomcat/websocket/TestPerMessageDeflate.java
> Modified:
>     tomcat/tc7.0.x/trunk/   (props changed)
>     tomcat/tc7.0.x/trunk/java/org/apache/tomcat/websocket/PerMessageDefla=
te.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 =3D uncompressedPart.getPayload();

> boolean fin =3D 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?

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=3Dtrue}

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.


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

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/PerMessag=
eDeflate.java
> URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/to=
mcat/websocket/PerMessageDeflate.java?rev=3D1807693&r1=3D1807692&r2=3D18076=
93&view=3Ddiff
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> --- tomcat/tc7.0.x/trunk/java/org/apache/tomcat/websocket/PerMessageDefla=
te.java (original)
> +++ tomcat/tc7.0.x/trunk/java/org/apache/tomcat/websocket/PerMessageDefla=
te.java Fri Sep  8 09:06:47 2017
> @@ -315,16 +315,20 @@ public class PerMessageDeflate implement
>      public List<MessagePart> sendMessagePart(List<MessagePart> uncompres=
sedParts) {
>          List<MessagePart> allCompressedParts =3D new ArrayList<MessagePa=
rt>();
>
> +        // Flag to track if a message is completely empty
> +        boolean emptyMessage =3D true;
> +
>          for (MessagePart uncompressedPart : uncompressedParts) {
>              byte opCode =3D uncompressedPart.getOpCode();
> +            boolean emptyPart =3D uncompressedPart.getPayload().limit() =
=3D=3D 0;
> +            emptyMessage =3D emptyMessage && emptyPart;
>              if (Util.isControl(opCode)) {
>                  // Control messages can appear in the middle of other me=
ssages
>                  // and must not be compressed. Pass it straight through
>                  allCompressedParts.add(uncompressedPart);
> -            } else if (uncompressedPart.getPayload().limit() =3D=3D 0 &&=
 uncompressedPart.isFin() &&
> -                    deflater.getBytesRead() =3D=3D 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 =3D new ArrayList<Mess=
agePart>();
>
> Copied: tomcat/tc7.0.x/trunk/test/org/apache/tomcat/websocket/TestPerMess=
ageDeflate.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/to=
mcat/websocket/TestPerMessageDeflate.java?p2=3Dtomcat/tc7.0.x/trunk/test/or=
g/apache/tomcat/websocket/TestPerMessageDeflate.java&p1=3Dtomcat/tc8.5.x/tr=
unk/test/org/apache/tomcat/websocket/TestPerMessageDeflate.java&r1=3D180768=
9&r2=3D1807693&rev=3D1807693&view=3Ddiff
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> --- tomcat/tc8.5.x/trunk/test/org/apache/tomcat/websocket/TestPerMessageD=
eflate.java (original)
> +++ tomcat/tc7.0.x/trunk/test/org/apache/tomcat/websocket/TestPerMessageD=
eflate.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 =3D Collections.emptyList();
> -        List<List<Parameter>> preferences =3D new ArrayList<>();
> +        List<List<Parameter>> preferences =3D new ArrayList<List<Paramet=
er>>();
>          preferences.add(parameters);
>
>          PerMessageDeflate perMessageDeflate =3D PerMessageDeflate.negoti=
ate(preferences, true);
>          perMessageDeflate.setNext(new TesterTransformation());
>
>          ByteBuffer bb1 =3D ByteBuffer.wrap("A".getBytes(StandardCharsets=
.UTF_8));
> -        MessagePart mp1 =3D new MessagePart(true, 0, Constants.OPCODE_TE=
XT, bb1, null, null, -1);
> +        MessagePart mp1 =3D new MessagePart(true, 0, Constants.OPCODE_TE=
XT, bb1, null, null);
>
> -        List<MessagePart> uncompressedParts1 =3D new ArrayList<>();
> +        List<MessagePart> uncompressedParts1 =3D new ArrayList<MessagePa=
rt>();
>          uncompressedParts1.add(mp1);
>          perMessageDeflate.sendMessagePart(uncompressedParts1);
>
>          ByteBuffer bb2 =3D ByteBuffer.wrap("".getBytes(StandardCharsets.=
UTF_8));
> -        MessagePart mp2 =3D new MessagePart(true, 0, Constants.OPCODE_TE=
XT, bb2, null, null, -1);
> +        MessagePart mp2 =3D new MessagePart(true, 0, Constants.OPCODE_TE=
XT, bb2, null, null);
>
> -        List<MessagePart> uncompressedParts2 =3D new ArrayList<>();
> +        List<MessagePart> uncompressedParts2 =3D new ArrayList<MessagePa=
rt>();
>          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/chang=
elog.xml?rev=3D1807693&r1=3D1807692&r2=3D1807693&view=3Ddiff
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> --- 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 2=
017
> @@ -100,6 +100,16 @@
>        </fix>
>      </changelog>
>    </subsection>
> +  <subsection name=3D"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=3D"Tribes">
>      <changelog>
>        <fix>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>

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