Bug 40960 - Timeout when reading from underlying socket does throw IOException with APR and SocketTimeoutException without APR
Summary: Timeout when reading from underlying socket does throw IOException with APR a...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 5
Classification: Unclassified
Component: Connector:HTTP (show other bugs)
Version: 5.5.22
Hardware: PC All
: P2 normal with 1 vote (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2006-11-13 10:22 UTC by Christophe Pierret
Modified: 2007-07-31 04:37 UTC (History)
0 users



Attachments
Patch incorporating inline suggested code change (1.04 KB, patch)
2006-12-29 12:09 UTC, Chris Halstead
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christophe Pierret 2006-11-13 10:22:30 UTC
Timeout when reading from underlying socket does throw IOException with APR and
SocketTimeoutException without APR.
This difference causes servlet code handling "timeout exceptions" to break (and
not be fixeable) if APR is used.
In class "org.apache.coyote.http11.InternalAprInputBuffer" for method "boolean
fill() throws IOException":
(in block if (!parsingHeader) ) The return code for Socket.recvbb  is not
checked for Status.ETIMEDOUT or Status.TIMEUP, therefore an IOException is
thrown whatever the reason for the exception.  This is a behavior change from
non-APR HTTP 1.1 connector.

The following new code for the fill method fixes the issue:
   /**
     * Fill the internal buffer using data from the undelying input stream.
     *
     * @return false if at end of stream
     */
    protected boolean fill()
        throws IOException {

        int nRead = 0;

        if (parsingHeader) {

            if (lastValid == buf.length) {
                throw new IOException
                    (sm.getString("iib.requestheadertoolarge.error"));
            }

            bbuf.clear();
            nRead = Socket.recvbb
                (socket, 0, buf.length - lastValid);
            if (nRead > 0) {
                bbuf.limit(nRead);
                bbuf.get(buf, pos, nRead);
                lastValid = pos + nRead;
            } else {
                if ((-nRead) == Status.EAGAIN) {
                    return false;
                } else {
                    throw new IOException(sm.getString("iib.failedread"));
                }
            }

        } else {

            buf = bodyBuffer;
            pos = 0;
            lastValid = 0;
            bbuf.clear();
            nRead = Socket.recvbb
                (socket, 0, buf.length);
            if (nRead > 0) {
                bbuf.limit(nRead);
                bbuf.get(buf, 0, nRead);
                lastValid = nRead;
            } else {
// CP: Start change
		if ((-nRead) == Status.ETIMEDOUT || (-nRead) == Status.TIMEUP) {
			throw new java.net.SocketTimeoutException(sm.getString("iib.failedread"));
		} else {
			throw new IOException(sm.getString("iib.failedread"));
		}
// CP: End change
            }

        }

        return (nRead > 0);

    }

Since SocketTimeOutException is also an IOException, this modification cannot
break code that does not filter timeout exceptions.
Comment 1 Chris Halstead 2006-12-29 12:09:23 UTC
Created attachment 19328 [details]
Patch incorporating inline suggested code change
Comment 2 Yoav Shapira 2007-03-25 09:21:23 UTC
You're right, this is a regression bug inconsistency, and I've run into it
myself.  I've just put in your patch on the 5.5 branch, in time for the next
release (5.5.24).  Thank you for contributing it.

It might also need to be applied in the Tomcat 6 tree, but I haven't checked and
there have been other relevant changes to the connector on that branch.
Comment 3 Christophe Pierret 2007-03-27 07:34:54 UTC
This one has been fixed in Tomcat6 using the same patch.
Comment 4 Sujai 2007-07-31 00:15:36 UTC
In the description it is told as the issue is there in 5.5.23,but in the
changelog of tomcat 5.5 it is been told that from the version 5.5.10 this issue
is fixed.

"Fix NPE when POST size exceeds limit defined by maxPostSize. (markt)".

Can anyone help me that what i refering is correct or not

(In reply to comment #0)
> Timeout when reading from underlying socket does throw IOException with APR and
> SocketTimeoutException without APR.
> This difference causes servlet code handling "timeout exceptions" to break (and
> not be fixeable) if APR is used.
> In class "org.apache.coyote.http11.InternalAprInputBuffer" for method "boolean
> fill() throws IOException":
> (in block if (!parsingHeader) ) The return code for Socket.recvbb  is not
> checked for Status.ETIMEDOUT or Status.TIMEUP, therefore an IOException is
> thrown whatever the reason for the exception.  This is a behavior change from
> non-APR HTTP 1.1 connector.
> 
> The following new code for the fill method fixes the issue:
>    /**
>      * Fill the internal buffer using data from the undelying input stream.
>      *
>      * @return false if at end of stream
>      */
>     protected boolean fill()
>         throws IOException {
> 
>         int nRead = 0;
> 
>         if (parsingHeader) {
> 
>             if (lastValid == buf.length) {
>                 throw new IOException
>                     (sm.getString("iib.requestheadertoolarge.error"));
>             }
> 
>             bbuf.clear();
>             nRead = Socket.recvbb
>                 (socket, 0, buf.length - lastValid);
>             if (nRead > 0) {
>                 bbuf.limit(nRead);
>                 bbuf.get(buf, pos, nRead);
>                 lastValid = pos + nRead;
>             } else {
>                 if ((-nRead) == Status.EAGAIN) {
>                     return false;
>                 } else {
>                     throw new IOException(sm.getString("iib.failedread"));
>                 }
>             }
> 
>         } else {
> 
>             buf = bodyBuffer;
>             pos = 0;
>             lastValid = 0;
>             bbuf.clear();
>             nRead = Socket.recvbb
>                 (socket, 0, buf.length);
>             if (nRead > 0) {
>                 bbuf.limit(nRead);
>                 bbuf.get(buf, 0, nRead);
>                 lastValid = nRead;
>             } else {
> // CP: Start change
> 		if ((-nRead) == Status.ETIMEDOUT || (-nRead) == Status.TIMEUP) {
> 			throw new java.net.SocketTimeoutException(sm.getString("iib.failedread"));
> 		} else {
> 			throw new IOException(sm.getString("iib.failedread"));
> 		}
> // CP: End change
>             }
> 
>         }
> 
>         return (nRead > 0);
> 
>     }
> 
> Since SocketTimeOutException is also an IOException, this modification cannot
> break code that does not filter timeout exceptions.

Comment 5 Mark Thomas 2007-07-31 04:37:42 UTC
(In reply to comment #4)
> In the description it is told as the issue is there in 5.5.23,but in the
> changelog of tomcat 5.5 it is been told that from the version 5.5.10 this issue
> is fixed.

You are mis-reading the changelog. This issue does not appear in the changelog
for 5.5.23.

> "Fix NPE when POST size exceeds limit defined by maxPostSize. (markt)".

The NPE fix has nothing to do with this bug.

The fix will be in 5.5.24 onwards.