Bug 54456

Summary: ChunkedInputFilter returning EOF when client closes connection without sending end chunk
Product: Tomcat 7 Reporter: Sudhan Moghe <sudhan.moghe>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Severity: normal    
Priority: P2    
Version: trunk   
Target Milestone: ---   
Hardware: PC   
OS: All   
Attachments: Patch to fix the bug

Description Sudhan Moghe 2013-01-21 16:45:51 UTC
Created attachment 29874 [details]
Patch to fix the bug

In my case client is processing user's InputStream and then sending data in chunks to server. At server end I read till EOF.
Client can be simple Java program or a web application. 

I am facing issue when client gets error on user's InputStream and closes connection with server. In that case ChunkedInputFilter is returning -1.
I have added logs and confirmed that endChunk is false.
Issue was fixed after I added following just before "return result;"

if (result == -1 && !endChunk)
			throw new EOFException("Unexpected end of stream while reading chunk body");

I think ChunkedInputFilter should return -1 only when it gets endChunk.

Also, from client end I am always sending complete chunk. So, in ChunkedInputFilter I should get EOF from socket stream while reading next chunk header (which is handled properly in ChunkedInputFilter) but that is not happening.

I have attached patch. Patch was generated against tomcat/tc7.0.x/trunk
Comment 1 Mark Thomas 2013-01-23 11:33:13 UTC
Which HTTP connector are you using? I'm having trouble reproducing this. I'm not against the patch but I'd like to understand what is going on first.
Comment 2 Mark Thomas 2013-01-23 19:49:00 UTC
Restoring the severity to something sensible.

Need requested information to progress this.
Comment 3 Sudhan Moghe 2013-02-01 17:33:25 UTC
I found the issue after we switched to protocol="HTTP/1.1".
But I have tested my patch with "HTTP/1.1" as well as NIO connector.
I am getting EOFException("Unexpected end of stream while reading chunk body") with both.
Client is sending 8kB chunks. The test cases that are failing send around 18mB data before closing connection.

Why I was not getting EOF in my servlet with NIO connector is something I need to check. ChunkedInputFilter is returning -1 for sure. I wanted to check that before giving update here but not getting time.
Will check and give details as soon as possible. May be some time next week.

One sure way of reproducing the issue is setting chunk size as 200 and sending only 100 bytes and then closing connection. 
My test case is not doing that and why is it happening in my case is something I need to check. I am sending complete 8kB chunks from client. In my case it should fail while reading header (which is handled properly).
Comment 4 Konstantin Kolinko 2013-02-05 22:34:41 UTC
The following lines in ChunkedInputFilter.java (162-164 in trunk)

        if (pos >= lastValid) {

is the only place in this file where the return value of "readBytes()" is not checked. 

Looking at the code, if the method returns -1 then (lastValid - pos) will be -1 and it will proceed with "chunk.setBytes(buf, pos, result);", which seems a wrong thing to do in such a case.

I have not tested what is the actual behaviour here, but an explicit "return -1" or "throw new EOFException()" would be better.
Comment 5 Konstantin Kolinko 2013-02-06 10:46:24 UTC
(In reply to comment #4)
> "throw new EOFException()" would be better.

On a thought, it would be better to use org.apache.catalina.connector.ClientAbortException or a generic IOException.

It is abnormal abort of connection by the client. It is not a proper end of stream.
Comment 6 Sudhan Moghe 2013-02-06 20:10:18 UTC
Tested with protocol="HTTP/1.1" and <async-supported>false</async-supported>

Took latest code from trunk and then added following debug statements to ChunkedInputFilter.

if (pos >= lastValid) {
        	int readBytes = readBytes();
			if (readBytes <= 0){
				System.out.println("ChunkedInputFilter.doRead()1 readBytes returned : " + readBytes);

        if (remaining > (lastValid - pos)) {
            result = lastValid - pos;
            System.out.println("ChunkedInputFilter.doRead()2 result: " + result + " remaining: " + remaining + " lastValid: " + lastValid + " pos: " + pos);

Output on console
ChunkedInputFilter.doRead()1 readBytes returned : -1
ChunkedInputFilter.doRead()2 result: -1 remaining: 8192 lastValid: 334 pos: 335

Method returns -1, which is treated as normal EOF. And application ends up processing incomplete data.

"return -1" won't do as that is what is being returned.

"throw new EOFException("Unexpected end of stream while reading chunk body");"
"throw new org.apache.catalina.connector.ClientAbortException()"
looks appropriate.
Comment 7 Mark Thomas 2013-02-07 11:53:14 UTC
(In reply to comment #5)
> On a thought, it would be better to use
> org.apache.catalina.connector.ClientAbortException or a generic IOException.

ClientAbortException is a disallowed import for org.apache.coyote. It will have to be IOException.
Comment 8 Mark Thomas 2013-02-07 11:58:09 UTC
Fixed in trunk and 7.0.x and will be included in 7.0.37 onwards.
Comment 9 Sudhan Moghe 2013-02-07 15:05:59 UTC
(In reply to comment #8)
> Fixed in trunk and 7.0.x and will be included in 7.0.37 onwards.
7.0.37 or 7.0.36?
Fix in 7.0.36 (if possible) would help a lot. We are already in production and can't use dev build.
Comment 10 Mark Thomas 2013-02-07 15:10:16 UTC
Which part of "will be included in 7.0.37 onwards" was unclear?
Comment 11 Sudhan Moghe 2013-02-07 15:19:17 UTC
Sorry for bothering you. I was just confirming as current release is 7.0.35.
Comment 12 Konstantin Kolinko 2013-02-09 14:43:14 UTC
I proposed backport of r1443430 to 6.0.
Comment 13 Konstantin Kolinko 2013-04-27 13:40:06 UTC
Fixed in Tomcat 6 by r1476544 , will be in 6.0.37.