Re: cvs commit: jakarta-tomcat-catalina/catalina/src/share/org/apache/coyote/tomcat5CoyoteRequest.java
Remy Maucherat wrote: Remy Maucherat wrote: Jean-Francois Arcand wrote: Can you build a test case for the issue ? That would be easier than trying to guess what's going on (and it must be an obvious bug if there's a bug, like for the input stream reading from the char buffer ...). You've got to be able to reproduce it without using JSPs or tags if it's a valid bug, right ? -1 about the NPE change. I don't want to add null checks in every method to handle the one broken webapp. This doesn't make any sense. OTOH, here's something I would accept if you want to do that cleanup: the clear() would associate the facade with a (static final) HTTPServletRequest implementation which would return an ISE for every method. And do the same for the response facade. I think it's not worth the pain, so I would be +0 for the change. I realized some time later that this doesn't work, as the response facade uses methods from CoyoteResponse (for the response commit state). Since I can't provide an acceptable solution, I'll have to accept your polish if you think it is useful, and I change my vote to 0. Remy - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: cvs commit: jakarta-tomcat-catalina/catalina/src/share/org/apache/coyote/tomcat5CoyoteRequest.java
Remy Maucherat wrote: Jean-Francois Arcand wrote: Yes, your explanation makes sence. I will look at the tag code to see what I can find.But the exception also occurs when tag pooling is set to off. If the tag has a bug/not well designed, I still think the app should not fail with an NPE but with an IllegalSomething exception.The CoyoteRequest object shouldn't be set to null when the client still have some reference to it. Can you build a test case for the issue ? That would be easier than trying to guess what's going on (and it must be an obvious bug if there's a bug, like for the input stream reading from the char buffer ...). You've got to be able to reproduce it without using JSPs or tags if it's a valid bug, right ? -1 about the NPE change. I don't want to add null checks in every method to handle the one broken webapp. This doesn't make any sense. OTOH, here's something I would accept if you want to do that cleanup: the clear() would associate the facade with a (static final) HTTPServletRequest implementation which would return an ISE for every method. And do the same for the response facade. I think it's not worth the pain, so I would be +0 for the change. Remy - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: cvs commit: jakarta-tomcat-catalina/catalina/src/share/org/apache/coyote/tomcat5CoyoteRequest.java
Jean-Francois Arcand wrote: Yes, your explanation makes sence. I will look at the tag code to see what I can find.But the exception also occurs when tag pooling is set to off. If the tag has a bug/not well designed, I still think the app should not fail with an NPE but with an IllegalSomething exception.The CoyoteRequest object shouldn't be set to null when the client still have some reference to it. Can you build a test case for the issue ? That would be easier than trying to guess what's going on (and it must be an obvious bug if there's a bug, like for the input stream reading from the char buffer ...). You've got to be able to reproduce it without using JSPs or tags if it's a valid bug, right ? -1 about the NPE change. I don't want to add null checks in every method to handle the one broken webapp. This doesn't make any sense. Remy - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: cvs commit: jakarta-tomcat-catalina/catalina/src/share/org/apache/coyote/tomcat5CoyoteRequest.java
[EMAIL PROTECTED] wrote: jfarcand2003/06/06 12:04:51 Modified:catalina/src/share/org/apache/coyote/tomcat5 CoyoteRequest.java Log: Revert the patch until I come with a better solution. I'd like to be convinced there's a bug here ;-) Look: When you access the request, getRequest will create a new facade if there's none. It will then be cleared and nulled only on recycle, which only occurs at the end of the request processing (or there's a bug). If your tag has incorrect pooling and keeps a reference, it could work very well without a security manager, but it's an accident (you're accessing a random underlying request). With a security manager, the request object becomes invalid after the request, and you get the NPE on the second request. The second request thing is a usual symptom of a bug with tag pooling. Do you have the source of the tag, BTW ? Remy - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: cvs commit: jakarta-tomcat-catalina/catalina/src/share/org/apache/coyote/tomcat5CoyoteRequest.java
Jean-Francois Arcand wrote:
OK, let's try to describe the problem. First, here is the stack trace
the application is throwing when running:
java.lang.NullPointerException
at
org.apache.coyote.tomcat5.CoyoteRequestFacade.getAttribute(CoyoteRequestFaca
de.java:271)
at com.sun.web.ui.view.html.CCButton.getAttribute(CCButton.java:306)
at
com.sun.web.ui.view.html.CCButton.restoreStateData(CCButton.java:284)
at com.sun.web.ui.view.html.CCButton.beginDisplay(CCButton.java:204)
at
com.sun.web.ui.taglib.html.CCButtonTag.getHTMLString(CCButtonTag.java:236)
at
com.sun.web.ui.taglib.html.CCButtonTag.doEndTag(CCButtonTag.java:178)
at
org.apache.jsp.Module3_jsp._jspx_meth_cc_button_0(Module3_jsp.java:205)
at
org.apache.jsp.Module3_jsp._jspx_meth_jato_form_0(Module3_jsp.java:138)
at
org.apache.jsp.Module3_jsp._jspx_meth_jato_useViewBean_0(Module3_jsp.java:97
)
at org.apache.jsp.Module3_jsp._jspService(Module3_jsp.java:70)
at org.apache.jasper.runtime.HttpJspBase.service(HttpJspBase.java:136)
at javax.servlet.http.HttpServlet.service(HttpServlet.java:856)
I don't think this is an application bug.
Note that the problem doesn't occurs with 4.0.x or if you run it without
the security manager. The first time the application runs (very simple
test), no exception is produced. The second time you run, then the old
facade object (the one used by the first request) is still available but
since the clear method has been invoked, the request instance is set to
null (so the NPE is thrown). It is clear that the facade object used
when the NPE is thrown is the one used by the first invokation. It seems
that facade object has not been recycled properly.
I can send you the war file if you want to see the behaviour. It is very
easy to reproduce on both 4.1.x and 5.0 (so that has nothing to do with
package protection/access and the security changes we made in 5).
I don't think the current solution open security issue, but I agree it
is not an elegant solution. The other solution we have is to not invoke,
in CoyoteRequest.recycle(), CoyoteRequestFacade.clear(), which set to
null the CoyoteRequest. That solution works also:
Index: CoyoteRequest.java
===
RCS file:
/home/cvs/jakarta-tomcat-catalina/catalina/src/share/org/apache/coyote/tomcat5/CoyoteRequest.java,v
retrieving revision 1.8
diff -u -r1.8 CoyoteRequest.java
--- CoyoteRequest.java 6 Jun 2003 03:03:33 - 1.8
+++ CoyoteRequest.java 6 Jun 2003 16:55:13 -
@@ -438,7 +438,6 @@
mappingData.recycle();
if ((Constants.SECURITY) && (facade != null)) {
-facade.clear();
facade = null;
}
.
This way the facade will be cleared and we will not create a facade for
every request.
If a webapp hags on to the facade, it can access the underlying request.
That seems to me like a security problem. That also seems like a
security bug to Bill, and that's why we both vetoed your commit.
I will look at the bug. However, since the facade is only cleared at the
end of the request processing (in the recycle), I don't see how you can
produce a valid test case.
It could be a bug with the tag and tag pooling. Disable tag pooling to
see if it fixes the problem.
Remy
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: cvs commit: jakarta-tomcat-catalina/catalina/src/share/org/apache/coyote/tomcat5CoyoteRequest.java
Bill Barker wrote: I'm -1 on this patch unless you can explain what the bug exactly was, and how the recycling couldn't properly reset the facade. I'm not really happy with the patch either. I'll postpone adding my (since it's the second, binding) -1 until you provide a better explaination. Well, I have no idea what the bug report mentioned looks like, so I can't provide a real evaluation. However, what I'm now pretty sure about is that the patch is possibly unsafe. Note: AFAIK, one -1 is enough. Remy - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: cvs commit: jakarta-tomcat-catalina/catalina/src/share/org/apache/coyote/tomcat5CoyoteRequest.java
[EMAIL PROTECTED] wrote:
jfarcand2003/06/05 20:03:33
Modified:catalina/src/share/org/apache/coyote/tomcat5
CoyoteRequest.java
Log:
When the SecurityManager is turned on, the facade is never properly garbaged. Bugtraq 48
66915 demonstrates a case where CoyoteRequestFacade is re-used with a request object equ
als to null (the getAttribute throws NPE). The bug also exists in Tomcat 4.1.x. (should
I port the patch?)
Also, the way response are recycled may also produce the same behaviour, althrough I can
't reproduce the exception.
Ok, I thought about it a bit more.
To give you more detail, the code in recycle() is:
if ((Constants.SECURITY) && (facade != null)) {
facade.clear();
facade = null;
}
Doing a clear forces the facade to dump its pointer to the request, so
you can't hold to the reference and invoke methods on it (you get NPEs).
What you did leads possibly to the creation of multiple facades, and
only the last one will be cleared. All others will still have a pointer
to the real request object, so could be used by a rogue thread to access
data randomly. As I don't quite remember if the last facade would be
given to the webapp (it could well be), your patch opens a potential
secuity problem.
To summarize, there's no point in facading for security (the security
manager will already protect from casting and method invocation) unless
you guarantee that all facades are disassociated from the underlying
request object.
So, I'm confirming my -1 for the patch.
Maybe you could post details on the bugs you're trying to fix. If it's
valid (I doubt it, all the similar NPEs I've seen were invalid), then
we'll need to find a safe solution.
Remy
-
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Re: cvs commit: jakarta-tomcat-catalina/catalina/src/share/org/apache/coyote/tomcat5CoyoteRequest.java
[EMAIL PROTECTED] wrote: jfarcand2003/06/05 20:03:33 Modified:catalina/src/share/org/apache/coyote/tomcat5 CoyoteRequest.java Log: When the SecurityManager is turned on, the facade is never properly garbaged. Bugtraq 48 66915 demonstrates a case where CoyoteRequestFacade is re-used with a request object equ als to null (the getAttribute throws NPE). The bug also exists in Tomcat 4.1.x. (should I port the patch?) Also, the way response are recycled may also produce the same behaviour, althrough I can 't reproduce the exception. I'm not sure I understand what was going on, and I have no access to bugtraq. I believe the NPE occurred because of an access beyond the useful lifecycle of the request. The facade should be set to null when recycling the request, so this is supposed to take care of the problem. BTW, there's no guarantee that getRequest will be called just once during the processing of the request. I'm -1 on this patch unless you can explain what the bug exactly was, and how the recycling couldn't properly reset the facade. Remy - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
