Bug 46984

Summary: Server incorrectly reports a 501 error on bad method name. Should report 400 error.
Product: Tomcat 5 Reporter: Jason Smith <jsmith>
Component: Connector:HTTPAssignee: Tomcat Developers Mailing List <dev>
Severity: minor    
Priority: P3    
Version: 5.5.27   
Target Milestone: ---   
Hardware: All   
OS: All   

Description Jason Smith 2009-04-07 07:44:46 UTC
I managed to send a corrupt data stream to Tomcat.  The result was that the method name turned out to be '0\n\n0\n\n0\n\nPOST'.  This was actually the method name that was returned to the servlet's .service(request, response) method by Tomcat.  The error message returned back to the client was:

 java.io.IOException: Server returned HTTP response code: 501 for URL: http://localhost/method-bug/bug
	at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1305)
	at sun.net.www.protocol.http.HttpURLConnection.getHeaderFields(HttpURLConnection.java:2187)

The 501 error indicates that the method is not implemented or not supported.  While this is somewhat true, there is a more applicable error message.  The 501 error is confusing, in that it indicates a valid method name was received.  In this case, the method name was formed with invalid characters (numbers and carriage returns). 

The correct behavior is that Tomcat should detect the bad/invalid/malformed method name and throw a 400 error, bad request.  

The offending code is in org.apache.coyote.http11.InternalInputBuffer.

The parseRequestLine code is naive.  It starts by skipping blank lines, then puts EVERYTHING up until the next 'space' character into the method name, including carriage returns, special characters, numbers, etc.  

Correct behavior is that it should detect an invalid method name (according to the spec) at this point, and throw error 400 (I think 400 is correct).

    public void parseRequestLine() throws IOException {
        int start = 0;
        // Skipping blank lines
        byte chr = 0;
        do {
            // Read new bytes if needed
            if (pos >= lastValid) {
                if (!fill())
                    throw new EOFException(sm.getString("iib.eof.error"));
            chr = buf[pos++];
        } while ((chr == Constants.CR) || (chr == Constants.LF));
        // Mark the current buffer position
        start = pos;
        // Reading the method name
        // Method name is always US-ASCII
        boolean space = false;
        while (!space) {
            // Read new bytes if needed
            if (pos >= lastValid) {
                if (!fill())
                    throw new EOFException(sm.getString("iib.eof.error"));
            ascbuf[pos] = (char) buf[pos];
            // Spec says single SP but it also says be tolerant of HT
            if (buf[pos] == Constants.SP || buf[pos] == Constants.HT) {
                space = true;
                request.method().setChars(ascbuf, start, pos - start);
Comment 1 Mark Thomas 2009-04-09 07:03:21 UTC
This has been fixed in trunk and proposed for 6.0.x and 5.5.x.
Comment 2 Mark Thomas 2009-05-02 18:29:41 UTC
This has been fixed in 6.0.x and will be included in 6.0.20 onwards.
Comment 3 Mark Thomas 2009-06-04 07:58:44 UTC
This has been fixed in 5.5.x and will be included in 5.5.28 onwards.