Re: Closing channel sockets

2018-01-12 Thread Rémy Maucherat 
On Fri, Jan 12, 2018 at 12:06 AM, Mark Thomas  wrote:

> Hi,
>
> I've been looking at how we close NIO channels and I think there is an
> opportunity for a little clean-up that, in turn, may allow a little
> de-duplication between NIO and NIO2.
>
> Currently, in various places in the codebase we close an NIO channel
> using some variation of:
>
> channel.socket().close();
> channel.close();
>
> My reading of the Javadoc, source code and some debugging suggests that
> these lines are equivalent and that we can simply do:
>
> channel.close();
>
> Across the codebase, you end up with a patch like this:
> http://people.apache.org/~markt/patches/2018-01-11-
> channel-close-tc9-v1.patch
>
> Before I apply this patch to trunk, can anyone see anything I am missing
> here? Is there a reason to keep the code as it is?
>
> Interesting attempt. As usual, the main reason to keep the code as is is:
it works :)

Rémy

Re: Http11OutputBuffer mixes write strategies

2018-01-12 Thread Mark Thomas 
On 11/01/18 23:12, Christopher Schultz wrote:



> If performance is a consideration, then most of the calls to write()
> should probably be calls to headerBuffer.put() because we can be
> (reasonably?) sure that writing "HTTP/1.1 " to the output buffer isn't
> going to overflow the buffer. Likewise with the bytes for the status
> code. With an omitted reason phrase, even the trailing SP CR LF can be
> collapsed into a single byte[] and written a single time to the
> ByteBuffer, instead of individually as the current code does.
> 
> If there is a reason to be inconsistent, let's state it and be
> consistently inconsistent. If there is no such reason, let's be
> consistent and either always call write() or always call
> headerBuffer.put().
> 
> Thoughts?

The length check in write includes an allowance for an additional 4
bytes. See BZ 57509 for background.

I agree that the additional checks (via write()) in sendStatus() are
unnecessary. If a user sets maxHttpHeaderSize so low you can't write the
status line then a BufferOverflowException seems perfectly reasonable.

I'm in favour of switching sendStatus() to use headerBuffer.put()
throughout, along with adding a comment to the effect that the buffer is
reset for each request so, unless maxHttpHeaderSize is set so low
nothing works, the status line will always fit so write it directly
without length checks.

Mark

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

Re: Closing channel sockets

2018-01-12 Thread Mark Thomas 
On 12/01/18 08:04, Rémy Maucherat wrote:
> On Fri, Jan 12, 2018 at 12:06 AM, Mark Thomas  wrote:
> 
>> Hi,
>>
>> I've been looking at how we close NIO channels and I think there is an
>> opportunity for a little clean-up that, in turn, may allow a little
>> de-duplication between NIO and NIO2.
>>
>> Currently, in various places in the codebase we close an NIO channel
>> using some variation of:
>>
>> channel.socket().close();
>> channel.close();
>>
>> My reading of the Javadoc, source code and some debugging suggests that
>> these lines are equivalent and that we can simply do:
>>
>> channel.close();
>>
>> Across the codebase, you end up with a patch like this:
>> http://people.apache.org/~markt/patches/2018-01-11-
>> channel-close-tc9-v1.patch
>>
>> Before I apply this patch to trunk, can anyone see anything I am missing
>> here? Is there a reason to keep the code as it is?
>>
> Interesting attempt. As usual, the main reason to keep the code as is is:
> it works :)

Fair point. Working is definitely a good thing :)

But, if the first line is unnecessary, then there are some small
benefits to removing it. I've run the unit tests (which give pretty good
coverage of I/O edge cases) and they all pass with the patch applied.

On the other hand, if the change is viewed as too risky, I'd be happy to
put it on the back-burner until we start thinking about Tomcat 10.

Mark


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

Re: JDK 10 entered Rampdown Phase One on 14th of December

2018-01-12 Thread Mark Thomas 
On 18/12/17 09:56, Rory O'Donnell wrote:



> *Feedback* - If you have suggestions or encounter bugs, please submit
> them using the usual Java SE bug-reporting channel.
> Be sure to include complete version information from the output of the
> |java --version| command.

Hi,

I did some testing on this today. I found a problem with our custom
LogManager triggered by some fixes to the JRE LogManager.

I've created a report via the standard mechanism as requested. The
internal review ID is 9052225. Anything you can do to get this in front
of the right folks to ensure it doesn't get overlooked would be appreciated.

Thanks,

Mark

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

[Bug 61992] New: DOS after "Error parsing HTTP request header" message

2018-01-12 Thread bugzilla 
https://bz.apache.org/bugzilla/show_bug.cgi?id=61992

Bug ID: 61992
   Summary: DOS after "Error parsing HTTP request header" message
   Product: Tomcat 7
   Version: 7.0.82
  Hardware: PC
OS: Linux
Status: NEW
  Severity: critical
  Priority: P2
 Component: Catalina
  Assignee: dev@tomcat.apache.org
  Reporter: apa...@resellerdesktop.de
  Target Milestone: ---

After this message : 


Jan 12, 2018 7:19:17 AM org.apache.coyote.http11.AbstractHttp11Processor
process
INFORMATION: Error parsing HTTP request header
 Note: further occurrences of HTTP header parsing errors will be logged at
DEBUG level.
java.lang.IllegalArgumentException: Invalid character found in method name.
HTTP method names must be tokens
at
org.apache.coyote.http11.InternalAprInputBuffer.parseRequestLine(InternalAprInputBuffer.java:185)
at
org.apache.coyote.http11.AbstractHttp11Processor.process(AbstractHttp11Processor.java:1028)
at
org.apache.coyote.AbstractProtocol$AbstractConnectionHandler.process(AbstractProtocol.java:637)
at
org.apache.tomcat.util.net.AprEndpoint$SocketWithOptionsProcessor.run(AprEndpoint.java:2492)
at
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at
org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
at java.lang.Thread.run(Thread.java:748)

Tomcat 7.0.82 stopped working. 

Processes were running. Requests to static content were processed, as those
entries show : 

Jan 12, 2018 7:22:01 AM org.apache.catalina.core.ApplicationContext log
INFORMATION: jsp: Error: /java/webapps/plattform/themes/TrueGrey/favicon.ico
(No such file or directory)
Jan 12, 2018 7:50:35 AM org.apache.catalina.core.ApplicationContext log
INFORMATION: jsp: Error: /java/webapps/plattform/themes/TrueGrey/favicon.ico
(No such file or directory)
Jan 12, 2018 8:16:30 AM org.apache.catalina.core.ApplicationContext log
INFORMATION: jsp: Error: /java/webapps/plattform/themes/TrueGrey/favicon.ico
(No such file or directory)
Jan 12, 2018 8:16:32 AM org.apache.catalina.core.ApplicationContext log
INFORMATION: jsp: Error: /java/webapps/plattform/themes/TrueGrey/favicon.ico
(No such file or directory)
Jan 12, 2018 8:16:41 AM org.apache.catalina.core.ApplicationContext log
INFORMATION: jsp: Error: /java/webapps/plattform/themes/normal/ambient (No such
file or directory)
Jan 12, 2018 8:17:41 AM org.apache.catalina.core.ApplicationContext log
INFORMATION: jsp: Error: /java/webapps/plattform/themes/normal/ambient (No such
file or directory)

But everything that included processing of dynamic pages as jsp, did not work
anymore.

Request did not get logged ( because they did not get processed to an defined
end ( endlessloop, my personal favorite ) :

O== Tomcat was running ...

java-1.8.0-openjdk-1.8.0.151-1.b12.fc26.i686
java-1.8.0-openjdk-headless-1.8.0.151-1.b12.fc26.i686
javapackages-tools-4.7.0-17.fc26.noarch
python3-javapackages-4.7.0-17.fc26.noarch
tzdata-java-2017c-1.fc26.noarch


Kernel 4.13.16-100.fc25.i686+PAE due to 4.14.11 fc26 ( the Spectre & Meltdown
patched kernel ) having extrem problems with all forms of java, not
specifically Tomcat.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 61992] DOS after "Error parsing HTTP request header" message

2018-01-12 Thread bugzilla 
https://bz.apache.org/bugzilla/show_bug.cgi?id=61992

Mark Thomas  changed:

   What|Removed |Added

 Status|NEW |NEEDINFO

--- Comment #1 from Mark Thomas  ---
That error message simple closes the current connection. Subsequent requests
will be processed normally.

Given that this message indicates a faulty client, it is more likely the client
continues to sent invalid requests which won't appear in the logs as DEBUG
level logging is not enabled by default.

We need more information to investigate this report. Steps to reproduce would
be ideal. A tcpdump and associated Tomcat logs for a series of failed requests
may be sufficient. Without further information this issue will eventually be
closed as WORKSFORME.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 61992] DOS after "Error parsing HTTP request header" message

2018-01-12 Thread bugzilla 
https://bz.apache.org/bugzilla/show_bug.cgi?id=61992

--- Comment #2 from Mark Thomas  ---
For completeness:

$telnet localhost 8080
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
GET/ / HTTP/1.1

HTTP/1.1 400 Bad Request
Server: Apache-Coyote/1.1
Transfer-Encoding: chunked
Date: Fri, 12 Jan 2018 11:14:48 GMT
Connection: close

0

Connection closed by foreign host.

Stacktrace in console.
Tomcat continued continued to process both static and dynamic requests
correctly.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

svn commit: r1820963 - in /tomcat/trunk: java/org/apache/coyote/http2/StreamProcessor.java webapps/docs/changelog.xml

2018-01-12 Thread remm 
Author: remm
Date: Fri Jan 12 11:19:37 2018
New Revision: 1820963

URL: http://svn.apache.org/viewvc?rev=1820963&view=rev
Log:
Fix order issue in the sendfile setup code.

Modified:
tomcat/trunk/java/org/apache/coyote/http2/StreamProcessor.java
tomcat/trunk/webapps/docs/changelog.xml

Modified: tomcat/trunk/java/org/apache/coyote/http2/StreamProcessor.java
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/StreamProcessor.java?rev=1820963&r1=1820962&r2=1820963&view=diff
==
--- tomcat/trunk/java/org/apache/coyote/http2/StreamProcessor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/StreamProcessor.java Fri Jan 12 
11:19:37 2018
@@ -116,8 +116,8 @@ class StreamProcessor extends AbstractPr
 String fileName = (String) stream.getCoyoteRequest().getAttribute(
 org.apache.coyote.Constants.SENDFILE_FILENAME_ATTR);
 if (fileName != null) {
-sendfileData.path = new File(fileName).toPath();
 sendfileData = new SendfileData();
+sendfileData.path = new File(fileName).toPath();
 sendfileData.pos = ((Long) stream.getCoyoteRequest().getAttribute(
 
org.apache.coyote.Constants.SENDFILE_FILE_START_ATTR)).longValue();
 sendfileData.end = ((Long) stream.getCoyoteRequest().getAttribute(

Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1820963&r1=1820962&r2=1820963&view=diff
==
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Fri Jan 12 11:19:37 2018
@@ -45,6 +45,13 @@
   issues do not "pop up" wrt. others).
 -->
 
+  
+
+  
+Fix NIO2 HTTP/2 sendfile. (remm)
+  
+
+  
   
 
   



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

svn commit: r1820964 - /tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java

2018-01-12 Thread remm 
Author: remm
Date: Fri Jan 12 11:21:41 2018
New Revision: 1820964

URL: http://svn.apache.org/viewvc?rev=1820964&view=rev
Log:
Fix remote host code with the APR connector. This issue is found only in this 
branch and connector.

Modified:
tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java

Modified: 
tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java
URL: 
http://svn.apache.org/viewvc/tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java?rev=1820964&r1=1820963&r2=1820964&view=diff
==
--- tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java 
(original)
+++ tomcat/tc8.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java 
Fri Jan 12 11:21:41 2018
@@ -323,9 +323,8 @@ public class Http11AprProcessor extends
 } catch (Exception e) {
 log.warn(sm.getString("http11processor.socket.info"), 
e);
 }
-} else {
-
request.remoteHost().setString(socketWrapper.getRemoteHost());
 }
+request.remoteHost().setString(socketWrapper.getRemoteHost());
 }
 break;
 }



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

Re: JDK 10 entered Rampdown Phase One on 14th of December

2018-01-12 Thread Rory O'Donnell 

Hi Mark,

Thanks for the feedback, I'll pass it on.

Rgds,Rory


On 12/01/2018 10:20, Mark Thomas wrote:

On 18/12/17 09:56, Rory O'Donnell wrote:




*Feedback* - If you have suggestions or encounter bugs, please submit
them using the usual Java SE bug-reporting channel.
Be sure to include complete version information from the output of the
|java --version| command.

Hi,

I did some testing on this today. I found a problem with our custom
LogManager triggered by some fixes to the JRE LogManager.

I've created a report via the standard mechanism as requested. The
internal review ID is 9052225. Anything you can do to get this in front
of the right folks to ensure it doesn't get overlooked would be appreciated.

Thanks,

Mark

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



--
Rgds,Rory O'Donnell
Quality Engineering Manager
Oracle EMEA , Dublin, Ireland


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

buildbot success in on tomcat-8-trunk

2018-01-12 Thread buildbot 
The Buildbot has detected a restored build on builder tomcat-8-trunk while 
building . Full details are available at:
https://ci.apache.org/builders/tomcat-8-trunk/builds/1220

Buildbot URL: https://ci.apache.org/

Buildslave for this Build: silvanus_ubuntu

Build Reason: The AnyBranchScheduler scheduler named 'on-tomcat-8-commit' 
triggered this build
Build Source Stamp: [branch tomcat/tc8.0.x/trunk] 1820964
Blamelist: remm

Build succeeded!

Sincerely,
 -The Buildbot




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

[Bug 61993] New: org.apache.tomcat.util.ByteChunk throws NegativeArray SizeException

2018-01-12 Thread bugzilla 
https://bz.apache.org/bugzilla/show_bug.cgi?id=61993

Bug ID: 61993
   Summary: org.apache.tomcat.util.ByteChunk throws NegativeArray
SizeException
   Product: Tomcat 7
   Version: 7.0.82
  Hardware: PC
OS: Linux
Status: NEW
  Keywords: Beginner, PatchAvailable, PortForward
  Severity: minor
  Priority: P2
 Component: Catalina
  Assignee: dev@tomcat.apache.org
  Reporter: davecrigh...@uk.ibm.com
  Target Milestone: ---

Created attachment 35675
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=35675&action=edit
If growing the buffer would overflow int then just set to max value

We use ByteChunk to read in the request body and we noticed 2 things.

1.) The length of ByteChunk is limited to MAX_INT so for our purposes we can
only process 2Gb requests before we need to alter our reading code.

2.) The way that ByteChunk grows means that in practice you can often only use
up to 1Gb of space.

The followign exception stack is thrown when append is called on and ByteChunk
over 1Gb in size:

java.lang.NegativeArraySizeException:java.lang.NegativeArraySizeException at
org.apache.tomcat.util.buf.ByteChunk.makeSpace(ByteChunk.java:527) at
org.apache.tomcat.util.buf.ByteChunk.append(ByteChunk.java:327)

Looking at issue number 1 it looks like a lot of classes would need to be
changed to use long but I do have a patch for a trivial check that allowed us
to process 2Gb requests.

Obviously this could silently truncate once you actually reach INT_MAX but it
seems from the previous check based on the limit that this is the desired
behaviour rather than to throw an exception.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: JDK 10 entered Rampdown Phase One on 14th of December

2018-01-12 Thread Rory O'Donnell 

Hi Mark

How serious is this issue ?

It's logged with a low priority, it might have to wait for JDK 11.

Rgds,Rory



On 12/01/2018 11:39, Rory O'Donnell wrote:

Hi Mark,

Thanks for the feedback, I'll pass it on.

Rgds,Rory


On 12/01/2018 10:20, Mark Thomas wrote:

On 18/12/17 09:56, Rory O'Donnell wrote:




*Feedback* - If you have suggestions or encounter bugs, please submit
them using the usual Java SE bug-reporting channel.
Be sure to include complete version information from the output of the
|java --version| command.

Hi,

I did some testing on this today. I found a problem with our custom
LogManager triggered by some fixes to the JRE LogManager.

I've created a report via the standard mechanism as requested. The
internal review ID is 9052225. Anything you can do to get this in front
of the right folks to ensure it doesn't get overlooked would be 
appreciated.


Thanks,

Mark

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





--
Rgds,Rory O'Donnell
Quality Engineering Manager
Oracle EMEA, Dublin,Ireland


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

[Bug 61993] org.apache.tomcat.util.ByteChunk throws NegativeArray SizeException

2018-01-12 Thread bugzilla 
https://bz.apache.org/bugzilla/show_bug.cgi?id=61993

Dave Crighton  changed:

   What|Removed |Added

 CC||davecrigh...@uk.ibm.com

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: JDK 10 entered Rampdown Phase One on 14th of December

2018-01-12 Thread Mark Thomas 
On 12/01/18 13:17, Rory O'Donnell wrote:
> Hi Mark
> 
> How serious is this issue ?

Hi,

In terms of functionality, it isn't serious. As far as I can tell, there
are no functional side-effects for Tomcat.

However, the more I think about this, the more serious an issue I think
this will be. A large percentage of Tomcat users operate in environments
where any logging that even hints that something isn't right has to be
addressed. A stack trace to stdout would certainly get their attention.

I think we need a way to suppress this message. Without it a large
proportional Tomcat users won't be allowed by organisational policy to
run Tomcat on JDK 10. "Trust us. You can ignore that stack trace, it is
a known issue with Java 10 that doesn't cause any harm." isn't going to
be enough for most users.

My preference remains making that method protected so we can override it
and make it a NO-OP (Tomcat's custom LogManager handles this already).
However, I'd settle for anything that enabled that message to be suppressed.

> It's logged with a low priority, it might have to wait for JDK 11.

Understood.

Cheers,

Mark


> 
> Rgds,Rory
> 
> 
> 
> On 12/01/2018 11:39, Rory O'Donnell wrote:
>> Hi Mark,
>>
>> Thanks for the feedback, I'll pass it on.
>>
>> Rgds,Rory
>>
>>
>> On 12/01/2018 10:20, Mark Thomas wrote:
>>> On 18/12/17 09:56, Rory O'Donnell wrote:
>>>
>>> 
>>>
 *Feedback* - If you have suggestions or encounter bugs, please submit
 them using the usual Java SE bug-reporting channel.
 Be sure to include complete version information from the output of the
 |java --version| command.
>>> Hi,
>>>
>>> I did some testing on this today. I found a problem with our custom
>>> LogManager triggered by some fixes to the JRE LogManager.
>>>
>>> I've created a report via the standard mechanism as requested. The
>>> internal review ID is 9052225. Anything you can do to get this in front
>>> of the right folks to ensure it doesn't get overlooked would be
>>> appreciated.
>>>
>>> Thanks,
>>>
>>> Mark
>>>
>>> -
>>> 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

svn commit: r1820981 - in /tomcat/trunk/java/org/apache/tomcat/util/buf: ByteChunk.java CharChunk.java

2018-01-12 Thread markt 
Author: markt
Date: Fri Jan 12 13:34:13 2018
New Revision: 1820981

URL: http://svn.apache.org/viewvc?rev=1820981&view=rev
Log:
Fix formatting. No functional change.

Modified:
tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java
tomcat/trunk/java/org/apache/tomcat/util/buf/CharChunk.java

Modified: tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java?rev=1820981&r1=1820980&r2=1820981&view=diff
==
--- tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java Fri Jan 12 
13:34:13 2018
@@ -546,38 +546,38 @@ public final class ByteChunk implements
 /**
  * Make space for len bytes. If len is small, allocate a reserve space too.
  * Never grow bigger than limit.
- * @param count The size
+ *
+ * @param count
+ *The size
  */
 public void makeSpace(int count) {
 byte[] tmp = null;
 
 int newSize;
-int desiredSize=end + count;
+int desiredSize = end + count;
 
 // Can't grow above the limit
-if( limit > 0 &&
-desiredSize > limit) {
-desiredSize=limit;
+if (limit > 0 && desiredSize > limit) {
+desiredSize = limit;
 }
 
-if( buff==null ) {
-if( desiredSize < 256 )
- {
-desiredSize=256; // take a minimum
+if (buff == null) {
+if (desiredSize < 256) {
+desiredSize = 256; // take a minimum
 }
-buff=new byte[desiredSize];
+buff = new byte[desiredSize];
 }
 
 // limit < buf.length ( the buffer is already big )
 // or we already have space XXX
-if( desiredSize <= buff.length ) {
+if (desiredSize <= buff.length) {
 return;
 }
 // grow in larger chunks
-if( desiredSize < 2 * buff.length ) {
-newSize= buff.length * 2;
+if (desiredSize < 2 * buff.length) {
+newSize = buff.length * 2;
 } else {
-newSize= buff.length * 2 + count ;
+newSize = buff.length * 2 + count;
 }
 
 if (limit > 0 && newSize > limit) {
@@ -585,11 +585,11 @@ public final class ByteChunk implements
 }
 tmp = new byte[newSize];
 
-System.arraycopy(buff, start, tmp, 0, end-start);
+System.arraycopy(buff, start, tmp, 0, end - start);
 buff = tmp;
 tmp = null;
-end=end-start;
-start=0;
+end = end - start;
+start = 0;
 }
 
 //  Conversion and getters 

Modified: tomcat/trunk/java/org/apache/tomcat/util/buf/CharChunk.java
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/buf/CharChunk.java?rev=1820981&r1=1820980&r2=1820981&view=diff
==
--- tomcat/trunk/java/org/apache/tomcat/util/buf/CharChunk.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/buf/CharChunk.java Fri Jan 12 
13:34:13 2018
@@ -415,41 +415,40 @@ public final class CharChunk implements
 }
 
 /**
- * Make space for len chars. If len is small, allocate
- * a reserve space too. Never grow bigger than limit.
- * @param count The size
+ * Make space for len chars. If len is small, allocate a reserve space too.
+ * Never grow bigger than limit.
+ *
+ * @param count
+ *The size
  */
-public void makeSpace(int count)
-{
+public void makeSpace(int count) {
 char[] tmp = null;
 
 int newSize;
-int desiredSize=end + count;
+int desiredSize = end + count;
 
 // Can't grow above the limit
-if( limit > 0 &&
-desiredSize > limit) {
-desiredSize=limit;
+if (limit > 0 && desiredSize > limit) {
+desiredSize = limit;
 }
 
-if( buff==null ) {
-if( desiredSize < 256 )
- {
-desiredSize=256; // take a minimum
+if (buff == null) {
+if (desiredSize < 256) {
+desiredSize = 256; // take a minimum
 }
-buff=new char[desiredSize];
+buff = new char[desiredSize];
 }
 
 // limit < buf.length ( the buffer is already big )
 // or we already have space XXX
-if( desiredSize <= buff.length) {
+if (desiredSize <= buff.length) {
 return;
 }
 // grow in larger chunks
-if( desiredSize < 2 * buff.length ) {
-newSize= buff.length * 2;
+if (desiredSize < 2 * buff.length) {
+newSize = buff.length * 2;
 } else {
-newSize=

svn commit: r1820994 - in /tomcat/trunk/java/org/apache/tomcat/util/buf: ByteChunk.java CharChunk.java

2018-01-12 Thread markt 
Author: markt
Date: Fri Jan 12 14:16:08 2018
New Revision: 1820994

URL: http://svn.apache.org/viewvc?rev=1820994&view=rev
Log:
Format classes. No functional change.

Modified:
tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java
tomcat/trunk/java/org/apache/tomcat/util/buf/CharChunk.java

Modified: tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java?rev=1820994&r1=1820993&r2=1820994&view=diff
==
--- tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java Fri Jan 12 
14:16:08 2018
@@ -43,16 +43,15 @@ import java.nio.charset.StandardCharsets
 // inside this way it could provide the search/etc on ByteBuffer, as a helper.
 
 /**
- * This class is used to represent a chunk of bytes, and
- * utilities to manipulate byte[].
+ * This class is used to represent a chunk of bytes, and utilities to 
manipulate
+ * byte[].
  *
  * The buffer can be modified and used for both input and output.
  *
  * There are 2 modes: The chunk can be associated with a sink - 
ByteInputChannel
  * or ByteOutputChannel, which will be used when the buffer is empty (on input)
- * or filled (on output).
- * For output, it can also grow. This operating mode is selected by calling
- * setLimit() or allocate(initial, limit) with limit != -1.
+ * or filled (on output). For output, it can also grow. This operating mode is
+ * selected by calling setLimit() or allocate(initial, limit) with limit != -1.
  *
  * Various search and append method are defined - similar with String and
  * StringBuffer, but operating on bytes.
@@ -71,11 +70,13 @@ public final class ByteChunk implements
 
 private static final long serialVersionUID = 1L;
 
-/** Input interface, used when the buffer is empty
+/**
+ * Input interface, used when the buffer is empty
  *
  * Same as java.nio.channel.ReadableByteChannel
  */
 public static interface ByteInputChannel {
+
 /**
  * Read new bytes.
  *
@@ -86,24 +87,26 @@ public final class ByteChunk implements
 public int realReadBytes() throws IOException;
 }
 
-/** Same as java.nio.channel.WritableByteChannel.
+/**
+ * Same as java.nio.channel.WritableByteChannel.
  */
 public static interface ByteOutputChannel {
+
 /**
- * Send the bytes ( usually the internal conversion buffer ).
- * Expect 8k output if the buffer is full.
+ * Send the bytes ( usually the internal conversion buffer ). Expect 8k
+ * output if the buffer is full.
  *
  * @param cbuf bytes that will be written
  * @param off offset in the bytes array
  * @param len length that will be written
  * @throws IOException If an I/O occurs while writing the bytes
  */
-public void realWriteBytes(byte cbuf[], int off, int len)
-throws IOException;
+public void realWriteBytes(byte cbuf[], int off, int len) throws 
IOException;
+
 
 /**
- * Send the bytes ( usually the internal conversion buffer ).
- * Expect 8k output if the buffer is full.
+ * Send the bytes ( usually the internal conversion buffer ). Expect 8k
+ * output if the buffer is full.
  *
  * @param from bytes that will be written
  * @throws IOException If an I/O occurs while writing the bytes
@@ -113,33 +116,35 @@ public final class ByteChunk implements
 
 // 
 
-/** Default encoding used to convert to strings. It should be UTF8,
-as most standards seem to converge, but the servlet API requires
-8859_1, and this object is used mostly for servlets.
-*/
+/**
+ * Default encoding used to convert to strings. It should be UTF8, as most
+ * standards seem to converge, but the servlet API requires 8859_1, and 
this
+ * object is used mostly for servlets.
+ */
 public static final Charset DEFAULT_CHARSET = StandardCharsets.ISO_8859_1;
 
-private int hashCode=0;
+private int hashCode = 0;
 // did we compute the hashcode ?
 private boolean hasHashCode = false;
 
 // byte[]
 private byte[] buff;
 
-private int start=0;
+private int start = 0;
 private int end;
 
 private transient Charset charset;
 
-private boolean isSet=false; // XXX
+private boolean isSet = false; // XXX
 
 // How much can it grow, when data is added
-private int limit=-1;
+private int limit = -1;
 
 // transient as serialization is primarily for values via, e.g. JMX
 private transient ByteInputChannel in = null;
 private transient ByteOutputChannel out = null;
 
+
 /**
  * Creates a new, uninitialized ByteChunk object.
  */
@@ -147,8 +152,9 @@ public final clas

[Bug 61993] org.apache.tomcat.util.ByteChunk throws NegativeArray SizeException

2018-01-12 Thread bugzilla 
https://bz.apache.org/bugzilla/show_bug.cgi?id=61993

--- Comment #1 from Mark Thomas  ---
Thanks for the report.

On closer inspection there appear to be a couple of other edge cases that could
be handled better.

I plan on putting together some unit tests to cover this case and the other
edge cases.

Note that CharChunk is likely to have similar issues.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 61993] org.apache.tomcat.util.ByteChunk throws NegativeArray SizeException

2018-01-12 Thread bugzilla 
https://bz.apache.org/bugzilla/show_bug.cgi?id=61993

--- Comment #2 from Dave Crighton  ---
Thanks Mark, 

I did consider submitting my own JUnit with the patch but it causes large
allocations which, at least for our own build systems, makes it unsuitable at
an operational level for automated unit test (requires higher heap sizes, slows
down the build etc).

Wasn't sure what the best practice for the project is.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: Closing channel sockets

2018-01-12 Thread Rémy Maucherat 
On Fri, Jan 12, 2018 at 9:27 AM, Mark Thomas  wrote:

> On 12/01/18 08:04, Rémy Maucherat wrote:
> > On Fri, Jan 12, 2018 at 12:06 AM, Mark Thomas  wrote:
> >
> >> Hi,
> >>
> >> I've been looking at how we close NIO channels and I think there is an
> >> opportunity for a little clean-up that, in turn, may allow a little
> >> de-duplication between NIO and NIO2.
> >>
> >> Currently, in various places in the codebase we close an NIO channel
> >> using some variation of:
> >>
> >> channel.socket().close();
> >> channel.close();
> >>
> >> My reading of the Javadoc, source code and some debugging suggests that
> >> these lines are equivalent and that we can simply do:
> >>
> >> channel.close();
> >>
> >> Across the codebase, you end up with a patch like this:
> >> http://people.apache.org/~markt/patches/2018-01-11-
> >> channel-close-tc9-v1.patch
> >>
> >> Before I apply this patch to trunk, can anyone see anything I am missing
> >> here? Is there a reason to keep the code as it is?
> >>
> > Interesting attempt. As usual, the main reason to keep the code as is is:
> > it works :)
>
> Fair point. Working is definitely a good thing :)
>
> But, if the first line is unnecessary, then there are some small
> benefits to removing it. I've run the unit tests (which give pretty good
> coverage of I/O edge cases) and they all pass with the patch applied.
>
> On the other hand, if the change is viewed as too risky, I'd be happy to
> put it on the back-burner until we start thinking about Tomcat 10.
>
> I have no issue with trying it.

Rémy

Re: Http11OutputBuffer mixes write strategies

2018-01-12 Thread Christopher Schultz 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Mark,

On 1/12/18 3:27 AM, Mark Thomas wrote:
> On 11/01/18 23:12, Christopher Schultz wrote:
> 
> 
> 
>> If performance is a consideration, then most of the calls to
>> write() should probably be calls to headerBuffer.put() because we
>> can be (reasonably?) sure that writing "HTTP/1.1 " to the output
>> buffer isn't going to overflow the buffer. Likewise with the
>> bytes for the status code. With an omitted reason phrase, even
>> the trailing SP CR LF can be collapsed into a single byte[] and
>> written a single time to the ByteBuffer, instead of individually
>> as the current code does.
>> 
>> If there is a reason to be inconsistent, let's state it and be 
>> consistently inconsistent. If there is no such reason, let's be 
>> consistent and either always call write() or always call 
>> headerBuffer.put().
>> 
>> Thoughts?
> 
> The length check in write includes an allowance for an additional
> 4 bytes. See BZ 57509 for background.
> 
> I agree that the additional checks (via write()) in sendStatus()
> are unnecessary. If a user sets maxHttpHeaderSize so low you can't
> write the status line then a BufferOverflowException seems
> perfectly reasonable.
> 
> I'm in favour of switching sendStatus() to use headerBuffer.put() 
> throughout, along with adding a comment to the effect that the
> buffer is reset for each request so, unless maxHttpHeaderSize is
> set so low nothing works, the status line will always fit so write
> it directly without length checks.

What do you think about a try/catch for BufferOverflowException that
re-throws the exception as HeadersTooLargeException? Would that
try/catch negatively impact performance enough to earn a -1 ? Since it
would be a hit exactly once per response (right?) it shouldn't be a
big deal, especially when offset by the savings of not re-checking the
buffer many times over (which is my proposal, here, of course).

- -chris
-BEGIN PGP SIGNATURE-
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQJRBAEBCAA7FiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlpZIXEdHGNocmlzQGNo
cmlzdG9waGVyc2NodWx0ei5uZXQACgkQHPApP6U8pFjbNg/8D8B5a26OIYQCtjUk
AhABuPWPXZmtCVyVIGtmi5r5bnHUZTj84KuxVGmIYLO2sGRaFnSfnCHRySaUwp1r
EEeB0DPAt1vE+TIPRyWJ/XqXFpCjBNid5bD0r09eAWgFR+kos1+wupiIE/91+Xwj
hsrC+pqi9Hst8dieEtKuwiakDYFjyQ8OqTv6ZQ3xce2bQr0Ua8H7RYh5lXkIDVIV
ChRcYTvC76twtTVXl0qvqEwe7UH7N4Y+ae73NTNvbCsUuq99iCr8YM3Hq4CqHnij
k8QTfSOaJ4wkvvnE6pac7pZcyPzA6eB8F1T8VapwqZB82bS3498bI1N4NJ7uLdH5
+6Ub6lWtwUvmpIFbiF/IBAliRked2xDC5lLmvp7ECzux3ytbLZs3wlKJKEr4IMvM
L25qHY3scjgS5fbmmTRu1wZR1NNQGDd2hUMOtIJMdgxqTW/kla+rYBnl0NS9qmRK
/zcWmOhXg5XpWaSYeTzXm5Gah7rezNHHTEy3AovXZ7rw1vvp9c5mcOrPv7rUrJP1
f3iEqUoY57dPdJmwcKx6hZTT7Pf6WZ13wAU3YNp+YEwci8hruRncYEHln06UIY7v
PdB7lq8tQjTOyr1b+ozDWGLuXKxiVLdHKk04H6TmZBr1pd7ldZxuj4RoW3gmtLqD
gwanLMxfx8mY0i23ghlJ0q5tDEs=
=K3Qq
-END PGP SIGNATURE-

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

Re: Closing channel sockets

2018-01-12 Thread Christopher Schultz 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Mark,

On 1/12/18 3:27 AM, Mark Thomas wrote:
> On 12/01/18 08:04, Rémy Maucherat wrote:
>> On Fri, Jan 12, 2018 at 12:06 AM, Mark Thomas 
>> wrote:
>> 
>>> Hi,
>>> 
>>> I've been looking at how we close NIO channels and I think
>>> there is an opportunity for a little clean-up that, in turn,
>>> may allow a little de-duplication between NIO and NIO2.
>>> 
>>> Currently, in various places in the codebase we close an NIO
>>> channel using some variation of:
>>> 
>>> channel.socket().close(); channel.close();
>>> 
>>> My reading of the Javadoc, source code and some debugging
>>> suggests that these lines are equivalent and that we can simply
>>> do:
>>> 
>>> channel.close();
>>> 
>>> Across the codebase, you end up with a patch like this: 
>>> http://people.apache.org/~markt/patches/2018-01-11- 
>>> channel-close-tc9-v1.patch
>>> 
>>> Before I apply this patch to trunk, can anyone see anything I
>>> am missing here? Is there a reason to keep the code as it is?
>>> 
>> Interesting attempt. As usual, the main reason to keep the code
>> as is is: it works :)
> 
> Fair point. Working is definitely a good thing :)
> 
> But, if the first line is unnecessary, then there are some small 
> benefits to removing it. I've run the unit tests (which give pretty
> good coverage of I/O edge cases) and they all pass with the patch
> applied.
> 
> On the other hand, if the change is viewed as too risky, I'd be
> happy to put it on the back-burner until we start thinking about
> Tomcat 10.

As Tomcat 9 is still unstable/beta, I'd be +1 for committing to Tomcat
9, but not back-porting to Tomcat 8/8.5. Less code (even if it's only
~30 lines source-wide) is always better, assuming equivalent behavior.

- -chris
-BEGIN PGP SIGNATURE-
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQJRBAEBCAA7FiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlpZIdcdHGNocmlzQGNo
cmlzdG9waGVyc2NodWx0ei5uZXQACgkQHPApP6U8pFgNnhAAt7OdJ8qFtc/bpE96
b6Rw3xIBZzAZ3RhL0Q4v2VMOoRtTSFf1y/44ElugIlJW4CmoGTiNxNgdrjWsQFqJ
9y2uQQP0TTSzGeDBqf+vPmkR8r0PdykhgofM3sOYGEZilkEg4YSTcdQRPtQGl2SS
GmtMQ7AL5GizgRshMuAJzTM0LL1/pWe5FTTGrGFZe/MmNOsbY0TjDEsM2kFVCulN
FVypyLdxRDDIWtZTxrBwd54fr7Z3r2JtrCPzPvy+pEKDrFetHl1xF83B5T0RzZTX
g5HIbRglNKkeh9XqGsCpgSbYa76UVSVqr4wu7Im0xaMGdQ71aFh6/d03cXVJzRfY
gONeBFX00nmkxuSj4Nl3s/snWPmONJqdN/c55EYYHDM0KBiSGWcV9bCGANNZs8RZ
vtuwGXd/KN6jfMl51h7Yuo67flL3VmRaXHPOx2a7VIxlmS5gzXoAbAcy/mU+9HYy
5KyjTt8QGiV5vNhHgEbo0O2xJ4d3+SKqvFb72bMqSH0kvnpA8ThA4eOcODJHlAH0
07yOual98utaHKkbcbgzqP8VLTLgTSyyJuWaWTWidUeBpzxlPezXE1XFXyhcm9tu
9CdsQ7S+9IMRo8+UsOHYJSV2R5UWXL5Hb63Kiinsfkr5tZGafeLudJPAVpn4WafA
mCd2cmld2FZp9cNYCohxjG2VbGI=
=5EiE
-END PGP SIGNATURE-

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

Re: Http11OutputBuffer mixes write strategies

2018-01-12 Thread Mark Thomas 
On 12/01/18 20:58, Christopher Schultz wrote:
> Mark,
> 
> On 1/12/18 3:27 AM, Mark Thomas wrote:
>> On 11/01/18 23:12, Christopher Schultz wrote:
> 
>> 
> 
>>> If performance is a consideration, then most of the calls to
>>> write() should probably be calls to headerBuffer.put() because we
>>> can be (reasonably?) sure that writing "HTTP/1.1 " to the output
>>> buffer isn't going to overflow the buffer. Likewise with the
>>> bytes for the status code. With an omitted reason phrase, even
>>> the trailing SP CR LF can be collapsed into a single byte[] and
>>> written a single time to the ByteBuffer, instead of individually
>>> as the current code does.
>>>
>>> If there is a reason to be inconsistent, let's state it and be 
>>> consistently inconsistent. If there is no such reason, let's be 
>>> consistent and either always call write() or always call 
>>> headerBuffer.put().
>>>
>>> Thoughts?
> 
>> The length check in write includes an allowance for an additional
>> 4 bytes. See BZ 57509 for background.
> 
>> I agree that the additional checks (via write()) in sendStatus()
>> are unnecessary. If a user sets maxHttpHeaderSize so low you can't
>> write the status line then a BufferOverflowException seems
>> perfectly reasonable.
> 
>> I'm in favour of switching sendStatus() to use headerBuffer.put() 
>> throughout, along with adding a comment to the effect that the
>> buffer is reset for each request so, unless maxHttpHeaderSize is
>> set so low nothing works, the status line will always fit so write
>> it directly without length checks.
> 
> What do you think about a try/catch for BufferOverflowException that
> re-throws the exception as HeadersTooLargeException? Would that
> try/catch negatively impact performance enough to earn a -1 ? Since it
> would be a hit exactly once per response (right?) it shouldn't be a
> big deal, especially when offset by the savings of not re-checking the
> buffer many times over (which is my proposal, here, of course).

I'm not -1 on a try/catch but I strongly prefer not having it. It the
user is daft enough to set maxHttpHeaderSize to less than the length of:
HTTP/1.1 nnn CRLF

then the deserve all they get 0 including an ugly exception. I'd prefer
the cleaner code.

Mark

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

[Bug 61977] JNDIRealm with SPNEGO, GSSAPI and SRV record fails to find LDAP SPN due to training sname period

2018-01-12 Thread bugzilla 
https://bz.apache.org/bugzilla/show_bug.cgi?id=61977

--- Comment #5 from marian.romasc...@nuance.com ---
Would it be possible to override in the webapp the JNDIRealm class in
catalina.jar with the patched version? I mean providing the class in a
webapp-specific jar. This taking advantage of Tomcat's class loader preference
for loading first the webapp classes. Just wondering - if there is a way to do
it the patch would be confined to the context of the webapp requiring the
removal of the trailing dot.
Not sure how this would be possible - kindly advise.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[Bug 61977] JNDIRealm with SPNEGO, GSSAPI and SRV record fails to find LDAP SPN due to training sname period

2018-01-12 Thread bugzilla 
https://bz.apache.org/bugzilla/show_bug.cgi?id=61977

--- Comment #6 from Mark Thomas  ---
Realm's many be specified at the Context, Host or Engine level. However, the
implementation class needs to visible to Tomcat so it needs to be in
CATALINA_BASE/lib.

So, to apply this patch to a single web app:
- create your patched Realm (e.g. extend the JNDIRealm and override open())
- add your class to CATALINA_BASE/lib
- configure the web app to use the Realm via the context.xml file for the app

-- 
You are receiving this mail because:
You are the assignee for the bug.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org