You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by bu...@apache.org on 2017/07/05 16:32:38 UTC
[Bug 61253] New: Tomcat's Digester silently ignore's failed property
replacement
https://bz.apache.org/bugzilla/show_bug.cgi?id=61253
Bug ID: 61253
Summary: Tomcat's Digester silently ignore's failed property
replacement
Product: Tomcat 8
Version: 8.5.x-trunk
Hardware: PC
OS: Linux
Status: NEW
Severity: normal
Priority: P2
Component: Util
Assignee: dev@tomcat.apache.org
Reporter: csutherl@redhat.com
Target Milestone: ----
I don't see much of a problem with this for vanilla tomcat, but if you're using
a PropertySource implementation with
org.apache.tomcat.util.digester.PROPERTY_SOURCE and it bombs somehow, the
Digester quietly eats the exception leaving the developer/user clueless.
Could we log a warn message in the catch block here
https://github.com/apache/tomcat85/blob/trunk/java/org/apache/tomcat/util/digester/Digester.java#L1990
saying that the replacement failed and that the property was not updated?
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 61253] Tomcat's Digester silently ignore's failed property
replacement
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61253
--- Comment #4 from Coty Sutherland <cs...@redhat.com> ---
Created attachment 35109
--> https://bz.apache.org/bugzilla/attachment.cgi?id=35109&action=edit
PropertySourceImpl to reproduce with
I didn't add a reproducer earlier, but here's one in case you guys want it...I
also noted after creating this reproducer that the only thing that could cause
the exception to be thrown are unchecked exceptions (in my case, an
IllegalArgumentException).
1) Copy the attached jar into lib
2) Configure catalina.properties to use it
org.apache.tomcat.util.digester.PROPERTY_SOURCE=org.apache.tomcat.example.digester.PropertySourceImpl
3) Add the trigger substitution to the server.xml so that the exception is
thrown (I couldn't just throw an exception because it made the return
unreachable)
$ sed -i 's@SHUTDOWN@${throwException}@' conf/server.xml
4) Starting or Stopping tomcat will reproduce the issue
WARNING: Attribute 1 failed to update and remains ${throwException}.
java.lang.IllegalArgumentException: Please don't ignore me tomcat :(
at
org.apache.tomcat.example.digester.PropertySourceImpl.getProperty(PropertySourceImpl.java:12)
at
org.apache.tomcat.util.IntrospectionUtils.replaceProperties(IntrospectionUtils.java:274)
at
org.apache.tomcat.util.digester.Digester.updateAttributes(Digester.java:1986)
at
org.apache.tomcat.util.digester.Digester.startElement(Digester.java:1170)
Note that in my case I added a warn to log it as I suggested
earlier...otherwise it get's ignored and you have to use a debugger to see the
problem :(
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 61253] Tomcat's Digester silently ignore's failed property
replacement
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61253
--- Comment #1 from Christopher Schultz <ch...@christopherschultz.net> ---
+1000
I believe that any empty catch block is a bug, unless it's one of those "never
happen" exceptions, and even those should be logged at an ERROR/FATAL level.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 61253] Tomcat's Digester silently ignore's failed property
replacement
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61253
--- Comment #5 from Coty Sutherland <cs...@redhat.com> ---
Created attachment 35123
--> https://bz.apache.org/bugzilla/attachment.cgi?id=35123&action=edit
Patch proposal that logs the attribute name and value that failed to update
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 61253] Tomcat's Digester silently ignore's failed property
replacement
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61253
Coty Sutherland <cs...@redhat.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--- Comment #6 from Coty Sutherland <cs...@redhat.com> ---
Fixed in:
- trunk for 9.0.0.M25 onwards
- 8.5.x for 8.5.19 onwards
- 8.0.x for 8.0.46 onwards
- 7.0.x for 7.0.80 onwards
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 61253] Tomcat's Digester silently ignore's failed property
replacement
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61253
--- Comment #2 from Konstantin Kolinko <kn...@gmail.com> ---
Bad idea.
The end user does not need to know that somebody tried a property substitution
here. It is not an error to write literal ${foo}. The correct substitution for
"${foo}" if foo does not exist is "${foo}" (no changes). There is no error
here.
The TLDs files in Standard taglib (JSTL) are an example of this: they have
documentation elements - examples - with a lot of ${}s in them. The examples
web application uses this library. You will see a lot of "failed substitution"
occurrences when running it. (I saw them when testing the patch for
CVE-2016-6794)
I do not mind if there is a debug logging for failed substitutions.
I think that logging a warning is a bad idea.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 61253] Tomcat's Digester silently ignore's failed property
replacement
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61253
Coty Sutherland <cs...@redhat.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Keywords| |Beginner
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 61253] Tomcat's Digester silently ignore's failed property
replacement
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61253
--- Comment #3 from Remy Maucherat <re...@apache.org> ---
As I see it, when a property doesn't exist (without errors) in the property
source (SystemPropertySource), it should return null and the substitution isn't
made. The source throwing an exception should mean there is an error. IMO it's
time to clarify the behavior (probably only in 8.5 and 9) and warn on possible
errors.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org