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

2017-09-08 Thread Mark Thomas 
On 08/09/17 13:23, Konstantin Kolinko wrote:
> 2017-09-08 12:06 GMT+03:00  :
>> 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 sendMessagePart(List 
>> uncompressedParts) {
>>  List allCompressedParts = new ArrayList();
>>
>> +// 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 compressedParts = new 
>> ArrayList();
>>
>> 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/websocke

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

2017-09-08 Thread Konstantin Kolinko 
2017-09-08 12:06 GMT+03:00  :
> 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?

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}

>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/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 sendMessagePart(List 
> uncompressedParts) {
>  List allCompressedParts = new ArrayList();
>
> +// 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 compressedParts = new 
> ArrayList();
>
> 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;
>

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

2017-09-08 Thread markt 
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

Propchange: tomcat/tc7.0.x/trunk/
--
--- svn:mergeinfo (original)
+++ svn:mergeinfo Fri Sep  8 09:06:47 2017
@@ -1,3 +1,3 @@
 
/tomcat/tc8.0.x/trunk:1636525,1637336,1637685,1637709,1638726,1640089,1640276,1640349,1640363,1640366,1640642,1640672,1640674,1640689,1640884,1641001,1641065,1641067,1641375,1641638,1641723,1641726,1641729-1641730,1641736,1641988,1642669-1642670,1642698,1642701,1643205,1643215,1643217,1643230,1643232,1643273,1643285,1643329-1643330,1643511,1643513,1643521,1643539,1643571,1643581-1643582,1643635,1643655,1643738,1643964,1644018,1644333,1644525,1644954,1644992,1645014,1645360,1645456,1645627,1645642,1645686,1645903-1645904,1645908-1645909,1645913,1645920,1646458,1646460-1646462,1646735,1646738-1646741,1646744,1646746,1646748-1646755,1646757,1646759-1646760,1647043,1648816,1651420-1651422,1651844,1652926,1652939-1652940,1652973,1653798,1653817,1653841,1654042,1654161,1654736,1654767,1654787,1656592,1659907,1662986,1663265,1663278,1663325,1663535,1663567,1663679,1663997,1664175,1664321,1664872,1665061,1665086,1666027,1666395,1666503,1666506,1666560,1666570,1666581,1666759,1666967,1666988
 
,1667553-1667555,1667558,1667617,1667633,1667637,1667747,1667767,1667873,1668028,1668137,1668634,1669432,1669801,1669840,1669895-1669896,1670398,1670435,1670592,1670605-1670607,1670609,1670632,1670720,1670725,1670727,1670731,1671114,1672273,1672285,1673759,1674220,1674295,1675469,1675488,1675595,1675831,1676232,1676367-1676369,1676382,1676394,1676483,1676556,1676635,1678178,1679536,1679988,1680256,1681124,1681182,1681703,1681730,1681840,1681864,1681869,1682010,1682034,1682047,1682052-1682053,1682062,1682064,1682070,1682312,1682325,1682331,1682386,1684367,1684385,1685759,1685774,1685827,1685892,1687341,1688904,1689358,1689657,1689921,1692850,1693093,1693108,1693324,1694060,1694115,1694291,1694427,1694431,1694503,1694549,1694789,1694873,1694881,1695356,1695372,1695823-1695825,1696200,1696281,1696379,1696468,1700608,1700871,1700897,1700978,1701094,1701124,1701608,1701668,1701676,1701766,1701944,1702248,1702252,1702314,1702390,1702723,1702725,1702728,1702730,1702733,1702735,1702737,1702
 
739,1702742,1702744,1702748,1702751,1702754,1702758,1702760,1702763,1702766,1708779,1708782,1708806,1709314,1709670,1710347,1710442,1710448,1710490,1710574,1710578,1712226,1712229,1712235,1712255,1712618,1712649,1712655,1712860,1712899,1712903,1712906,1712913,1712926,1712975,1713185,1713262,1713287,1713613,1713621,1713872,1713976,1713994,1713998,1714004,1714013,1714059,1714538,1714580,1715189,1715207,1715544,1715549,1715637,1715639-1715645,1715667,1715683,1715866,1715978,1715981,1716216-1716217,1716355,1716414,1716421,1717208-1717209,1717257,1717283,1717288,1717291,1717421,1717517,1717529,1718797,1718840-1718843,1719348,1719357-1719358,1719400,1719491,1719737,1720235,1720396,1720442,1720446,1720450,1720463,1720658-1720660,1720756,1720816,1721813,1721818,1721831,1721861,1721867,1721882,1722523,1722527,1722800,1722926,1722941,1722997,1723130,1723440,1723488,1723890,1724434,1724674,1724792,1724803,1724902,1725128,1725131,1725154,1725167,1725911,1725921,1725929,1725963-1725965,1725970,1
 
725974,1726171-1726173,1726175,1726179-1726182,1726190-1726191,1726195-1726200,1726203,1726226,1726576,1726630,1726992,1727029,1727037,1727671,1727676,1727900,1728028,1728092,1728439,1728449,1729186,1729362,1731009,1731303,1731867,1731872,1731874,1731876,1731885,1731947,1731955,1731959,1731977,1731984,1732360,1732490,1732672,1732902,1733166,1733603,1733619,1733735,1733752,1733764,1733915,1733941,1733964,1734115,1734133,1734261,1734421,1734531,1736286,1737967,1738173,1738182,1738992,1739039,1739089-1739091,1739294,1739777,1739821,1739981,1740513,1740726,1741019,1741162,1741217,1743647,1743681,1744152,1744272,1746732,1746750,1752739,1754615,1755886,1756018,1759565,1761686,1762173,1762206,1766280,1767507-1767508,1767653,1767656,1769267,1772949,1773521,1773527,1774104,1777015,1777213,1779330,1783151,1784188,1784966,1785670,1786846,1788260,1788999,1789140,1789402,1791529,1791559,1795291,1796906,1797523,1799214,1800998-1800999,1801003,1801007-1801008,1801017,1801020,1802808,1802814,180361
 8,1806107,1806733,1807082-1807083
-/tomcat/tc8.5.x/tr