You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by kk...@apache.org on 2011/10/19 09:15:30 UTC

svn commit: r1185998 - in /tomcat/tc6.0.x/trunk: ./ java/org/apache/catalina/connector/ java/org/apache/coyote/ajp/ java/org/apache/coyote/http11/ webapps/docs/

Author: kkolinko
Date: Wed Oct 19 07:15:30 2011
New Revision: 1185998

URL: http://svn.apache.org/viewvc?rev=1185998&view=rev
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=51872
Ensure access log always logs the correct remote IP.
Ensure requests with multiple errors do not result in multiple access log
entries.

Modified:
    tomcat/tc6.0.x/trunk/STATUS.txt
    tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
    tomcat/tc6.0.x/trunk/java/org/apache/coyote/ajp/AjpAprProcessor.java
    tomcat/tc6.0.x/trunk/java/org/apache/coyote/ajp/AjpProcessor.java
    tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java
    tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java
    tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11Processor.java
    tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml

Modified: tomcat/tc6.0.x/trunk/STATUS.txt
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=1185998&r1=1185997&r2=1185998&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/STATUS.txt (original)
+++ tomcat/tc6.0.x/trunk/STATUS.txt Wed Oct 19 07:15:30 2011
@@ -64,14 +64,6 @@ PATCHES PROPOSED TO BACKPORT:
       - getStuckThreadIds() returns a list of ids. It might be useful to
         have a similar method that returns Thread.getName() names.
 
-* Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=51872
-  Ensure access log always logs the correct remote IP.
-  Ensure requests with multiple errors do not result in multiple access log
-  entries.
-  http://people.apache.org/~markt/patches/2011-09-27-bug51872-tc6.patch
-  +1: markt, kkolinko, rjung
-  -1:
-
 * Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=51905
   Fix infinite loop in AprEndpoint shutdown if acceptor unlock
   fails. Reduce timeout before forcefully closing the socket from 30s to 10s.

Modified: tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java?rev=1185998&r1=1185997&r2=1185998&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java (original)
+++ tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java Wed Oct 19 07:15:30 2011
@@ -24,6 +24,7 @@ import java.io.UnsupportedEncodingExcept
 import org.apache.catalina.CometEvent;
 import org.apache.catalina.Context;
 import org.apache.catalina.Globals;
+import org.apache.catalina.Host;
 import org.apache.catalina.Wrapper;
 import org.apache.catalina.util.StringManager;
 import org.apache.catalina.util.ServerInfo;
@@ -32,6 +33,7 @@ import org.apache.coyote.ActionCode;
 import org.apache.coyote.Adapter;
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
+import org.apache.tomcat.util.ExceptionUtils;
 import org.apache.tomcat.util.buf.B2CConverter;
 import org.apache.tomcat.util.buf.ByteChunk;
 import org.apache.tomcat.util.buf.CharChunk;
@@ -340,10 +342,8 @@ public class CoyoteAdapter implements Ad
 
         Request request = (Request) req.getNote(ADAPTER_NOTES);
         Response response = (Response) res.getNote(ADAPTER_NOTES);
-        boolean create = false;
 
         if (request == null) {
-            create = true;
             // Create objects
             request = connector.createRequest();
             request.setCoyoteRequest(req);
@@ -363,10 +363,29 @@ public class CoyoteAdapter implements Ad
                 (connector.getURIEncoding());
         }
         
-        connector.getService().getContainer().logAccess(
-                request, response, time, true);
-        
-        if (create) {
+        try {
+            // Log at the lowest level available. logAccess() will be
+            // automatically called on parent containers.
+            boolean logged = false;
+            if (request.mappingData != null) {
+                if (request.mappingData.context != null) {
+                    logged = true;
+                    ((Context) request.mappingData.context).logAccess(
+                            request, response, time, true);
+                } else if (request.mappingData.host != null) {
+                    logged = true;
+                    ((Host) request.mappingData.host).logAccess(
+                            request, response, time, true);
+                }
+            }
+            if (!logged) {
+                connector.getService().getContainer().logAccess(
+                        request, response, time, true);
+            }
+        } catch (Throwable t) {
+            ExceptionUtils.handleThrowable(t);
+            log.warn(sm.getString("coyoteAdapter.accesslogFail"), t);
+        } finally {
             request.recycle();
             response.recycle();
         }

Modified: tomcat/tc6.0.x/trunk/java/org/apache/coyote/ajp/AjpAprProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/coyote/ajp/AjpAprProcessor.java?rev=1185998&r1=1185997&r2=1185998&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/java/org/apache/coyote/ajp/AjpAprProcessor.java (original)
+++ tomcat/tc6.0.x/trunk/java/org/apache/coyote/ajp/AjpAprProcessor.java Wed Oct 19 07:15:30 2011
@@ -428,15 +428,17 @@ public class AjpAprProcessor implements 
             }
 
             // Setting up filters, and parse some request headers
-            rp.setStage(org.apache.coyote.Constants.STAGE_PREPARE);
-            try {
-                prepareRequest();
-            } catch (Throwable t) {
-                log.debug(sm.getString("ajpprocessor.request.prepare"), t);
-                // 400 - Internal Server Error
-                response.setStatus(400);
-                adapter.log(request, response, 0);
-                error = true;
+            if (!error) {
+                rp.setStage(org.apache.coyote.Constants.STAGE_PREPARE);
+                try {
+                    prepareRequest();
+                } catch (Throwable t) {
+                    log.debug(sm.getString("ajpprocessor.request.prepare"), t);
+                    // 400 - Internal Server Error
+                    response.setStatus(400);
+                    adapter.log(request, response, 0);
+                    error = true;
+                }
             }
 
             // Process the request in the adapter
@@ -839,7 +841,6 @@ public class AjpAprProcessor implements 
                     secret = true;
                     if (!tmpMB.equals(requiredSecret)) {
                         response.setStatus(403);
-                        adapter.log(request, response, 0);
                         error = true;
                     }
                 }
@@ -856,7 +857,6 @@ public class AjpAprProcessor implements 
         // Check if secret was submitted if required
         if ((requiredSecret != null) && !secret) {
             response.setStatus(403);
-            adapter.log(request, response, 0);
             error = true;
         }
 
@@ -890,6 +890,9 @@ public class AjpAprProcessor implements 
         MessageBytes valueMB = request.getMimeHeaders().getValue("host");
         parseHost(valueMB);
 
+        if (error) {
+            adapter.log(request, response, 0);
+        }
     }
 
 
@@ -905,7 +908,6 @@ public class AjpAprProcessor implements 
                 request.serverName().duplicate(request.localName());
             } catch (IOException e) {
                 response.setStatus(400);
-                adapter.log(request, response, 0);
                 error = true;
             }
             return;
@@ -957,7 +959,6 @@ public class AjpAprProcessor implements 
                     error = true;
                     // 400 - Bad request
                     response.setStatus(400);
-                    adapter.log(request, response, 0);
                     break;
                 }
                 port = port + (charValue * mult);

Modified: tomcat/tc6.0.x/trunk/java/org/apache/coyote/ajp/AjpProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/coyote/ajp/AjpProcessor.java?rev=1185998&r1=1185997&r2=1185998&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/java/org/apache/coyote/ajp/AjpProcessor.java (original)
+++ tomcat/tc6.0.x/trunk/java/org/apache/coyote/ajp/AjpProcessor.java Wed Oct 19 07:15:30 2011
@@ -445,15 +445,17 @@ public class AjpProcessor implements Act
             }
 
             // Setting up filters, and parse some request headers
-            rp.setStage(org.apache.coyote.Constants.STAGE_PREPARE);
-            try {
-                prepareRequest();
-            } catch (Throwable t) {
-                log.debug(sm.getString("ajpprocessor.request.prepare"), t);
-                // 400 - Internal Server Error
-                response.setStatus(400);
-                adapter.log(request, response, 0);
-                error = true;
+            if (!error) {
+                rp.setStage(org.apache.coyote.Constants.STAGE_PREPARE);
+                try {
+                    prepareRequest();
+                } catch (Throwable t) {
+                    log.debug(sm.getString("ajpprocessor.request.prepare"), t);
+                    // 400 - Internal Server Error
+                    response.setStatus(400);
+                    adapter.log(request, response, 0);
+                    error = true;
+                }
             }
 
             // Process the request in the adapter
@@ -844,7 +846,6 @@ public class AjpProcessor implements Act
                     secret = true;
                     if (!tmpMB.equals(requiredSecret)) {
                         response.setStatus(403);
-                        adapter.log(request, response, 0);
                         error = true;
                     }
                 }
@@ -861,7 +862,6 @@ public class AjpProcessor implements Act
         // Check if secret was submitted if required
         if ((requiredSecret != null) && !secret) {
             response.setStatus(403);
-            adapter.log(request, response, 0);
             error = true;
         }
 
@@ -895,6 +895,9 @@ public class AjpProcessor implements Act
         MessageBytes valueMB = request.getMimeHeaders().getValue("host");
         parseHost(valueMB);
 
+        if (error) {
+            adapter.log(request, response, 0);
+        }
     }
 
 
@@ -910,7 +913,6 @@ public class AjpProcessor implements Act
                 request.serverName().duplicate(request.localName());
             } catch (IOException e) {
                 response.setStatus(400);
-                adapter.log(request, response, 0);
                 error = true;
             }
             return;
@@ -962,7 +964,6 @@ public class AjpProcessor implements Act
                     error = true;
                     // 400 - Bad request
                     response.setStatus(400);
-                    adapter.log(request, response, 0);
                     break;
                 }
                 port = port + (charValue * mult);

Modified: tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java?rev=1185998&r1=1185997&r2=1185998&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java (original)
+++ tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java Wed Oct 19 07:15:30 2011
@@ -972,8 +972,9 @@ public class Http11AprProcessor implemen
         } catch (Throwable t) {
             log.error(sm.getString("http11processor.request.finish"), t);
             // 500 - Internal Server Error
+            // Can't add a 500 to the access log since that has already been
+            // written in the Adapter.service method.
             response.setStatus(500);
-            adapter.log(request, response, 0);
             error = true;
         }
         try {
@@ -1327,7 +1328,6 @@ public class Http11AprProcessor implemen
             error = true;
             // Send 505; Unsupported HTTP version
             response.setStatus(505);
-            adapter.log(request, response, 0);
         }
 
         MessageBytes methodMB = request.method();
@@ -1425,7 +1425,6 @@ public class Http11AprProcessor implemen
                     error = true;
                     // 501 - Unimplemented
                     response.setStatus(501);
-                    adapter.log(request, response, 0);
                 }
                 startPos = commaPos + 1;
                 commaPos = transferEncodingValue.indexOf(',', startPos);
@@ -1437,7 +1436,6 @@ public class Http11AprProcessor implemen
                 error = true;
                 // 501 - Unimplemented
                 response.setStatus(501);
-                adapter.log(request, response, 0);
             }
         }
 
@@ -1456,7 +1454,6 @@ public class Http11AprProcessor implemen
             error = true;
             // 400 - Bad request
             response.setStatus(400);
-            adapter.log(request, response, 0);
         }
 
         parseHost(valueMB);
@@ -1476,7 +1473,10 @@ public class Http11AprProcessor implemen
         }
         // Advertise comet support through a request attribute
         request.setAttribute("org.apache.tomcat.comet.support", Boolean.TRUE);
-        
+
+        if (error) {
+            adapter.log(request, response, 0);
+        }
     }
 
 
@@ -1539,7 +1539,6 @@ public class Http11AprProcessor implemen
                     error = true;
                     // 400 - Bad request
                     response.setStatus(400);
-                    adapter.log(request, response, 0);
                     break;
                 }
                 port = port + (charValue * mult);

Modified: tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java?rev=1185998&r1=1185997&r2=1185998&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java (original)
+++ tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java Wed Oct 19 07:15:30 2011
@@ -986,8 +986,9 @@ public class Http11NioProcessor implemen
         } catch (Throwable t) {
             log.error(sm.getString("http11processor.request.finish"), t);
             // 500 - Internal Server Error
+            // Can't add a 500 to the access log since that has already been
+            // written in the Adapter.service method.
             response.setStatus(500);
-            adapter.log(request, response, 0);
             error = true;
         }
         try {
@@ -1322,7 +1323,6 @@ public class Http11NioProcessor implemen
             error = true;
             // Send 505; Unsupported HTTP version
             response.setStatus(505);
-            adapter.log(request, response, 0);
         }
 
         MessageBytes methodMB = request.method();
@@ -1420,7 +1420,6 @@ public class Http11NioProcessor implemen
                     error = true;
                     // 501 - Unimplemented
                     response.setStatus(501);
-                    adapter.log(request, response, 0);
                 }
                 startPos = commaPos + 1;
                 commaPos = transferEncodingValue.indexOf(',', startPos);
@@ -1432,7 +1431,6 @@ public class Http11NioProcessor implemen
                 error = true;
                 // 501 - Unimplemented
                 response.setStatus(501);
-                adapter.log(request, response, 0);
             }
         }
 
@@ -1451,7 +1449,6 @@ public class Http11NioProcessor implemen
             error = true;
             // 400 - Bad request
             response.setStatus(400);
-            adapter.log(request, response, 0);
         }
 
         parseHost(valueMB);
@@ -1473,6 +1470,9 @@ public class Http11NioProcessor implemen
         // Advertise comet timeout support
         request.setAttribute("org.apache.tomcat.comet.timeout.support", Boolean.TRUE);
 
+        if (error) {
+            adapter.log(request, response, 0);
+        }
     }
 
 
@@ -1535,7 +1535,6 @@ public class Http11NioProcessor implemen
                     error = true;
                     // 400 - Bad request
                     response.setStatus(400);
-                    adapter.log(request, response, 0);
                     break;
                 }
                 port = port + (charValue * mult);

Modified: tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11Processor.java
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11Processor.java?rev=1185998&r1=1185997&r2=1185998&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11Processor.java (original)
+++ tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/Http11Processor.java Wed Oct 19 07:15:30 2011
@@ -893,7 +893,7 @@ public class Http11Processor implements 
                 log.error(sm.getString("http11processor.request.finish"), t);
                 // 500 - Internal Server Error
                 response.setStatus(500);
-                adapter.log(request, response, 0);
+                // No access logging since after service method 
                 error = true;
             }
             try {
@@ -1201,7 +1201,6 @@ public class Http11Processor implements 
                           " Unsupported HTTP version \""+protocolMB+"\"");
             }
             response.setStatus(505);
-            adapter.log(request, response, 0);
         }
 
         MessageBytes methodMB = request.method();
@@ -1299,7 +1298,6 @@ public class Http11Processor implements 
                     error = true;
                     // 501 - Unimplemented
                     response.setStatus(501);
-                    adapter.log(request, response, 0);
                 }
                 startPos = commaPos + 1;
                 commaPos = transferEncodingValue.indexOf(',', startPos);
@@ -1315,7 +1313,6 @@ public class Http11Processor implements 
                               " Unsupported transfer encoding \""+encodingName+"\"");
                 }
                 response.setStatus(501);
-                adapter.log(request, response, 0);
             }
         }
 
@@ -1338,7 +1335,6 @@ public class Http11Processor implements 
                           " host header missing");
             }
             response.setStatus(400);
-            adapter.log(request, response, 0);
         }
 
         parseHost(valueMB);
@@ -1352,6 +1348,9 @@ public class Http11Processor implements 
             contentDelimitation = true;
         }
 
+        if (error) {
+            adapter.log(request, response, 0);
+        }
     }
 
 
@@ -1418,7 +1417,6 @@ public class Http11Processor implements 
                     error = true;
                     // 400 - Bad request
                     response.setStatus(400);
-                    adapter.log(request, response, 0);
                     break;
                 }
                 port = port + (charValue * mult);

Modified: tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml?rev=1185998&r1=1185997&r2=1185998&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml Wed Oct 19 07:15:30 2011
@@ -66,6 +66,12 @@
         <code>JreMemoryLeakPreventionListener</code> to allow pre-loading of configurable
         classes to avoid some classloader leaks. (slaurent)
       </add>
+      <fix>
+        <bug>51872</bug>: Ensure that the access log always uses the correct
+        value for the remote IP address associated with the request and that
+        requests with multiple errors do not result in multiple entries in
+        the access log. (markt)
+      </fix>
       <add>
         Allow to overwrite the check for distributability
         of session attributes by session implementations. (rjung)



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