Bug 61253 - Tomcat's Digester silently ignore's failed property replacement
Summary: Tomcat's Digester silently ignore's failed property replacement
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Util (show other bugs)
Version: 8.5.x-trunk
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords: Beginner
Depends on:
Blocks:
 
Reported: 2017-07-05 16:32 UTC by Coty Sutherland
Modified: 2017-07-21 19:52 UTC (History)
0 users



Attachments
PropertySourceImpl to reproduce with (1.19 KB, application/x-java-archive)
2017-07-10 15:56 UTC, Coty Sutherland
Details
Patch proposal that logs the attribute name and value that failed to update (610 bytes, patch)
2017-07-12 12:13 UTC, Coty Sutherland
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Coty Sutherland 2017-07-05 16:32:38 UTC
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?
Comment 1 Christopher Schultz 2017-07-05 17:17:35 UTC
+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.
Comment 2 Konstantin Kolinko 2017-07-05 18:04:21 UTC
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.
Comment 3 Remy Maucherat 2017-07-06 13:51:31 UTC
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.
Comment 4 Coty Sutherland 2017-07-10 15:56:37 UTC
Created attachment 35109 [details]
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 :(
Comment 5 Coty Sutherland 2017-07-12 12:13:07 UTC
Created attachment 35123 [details]
Patch proposal that logs the attribute name and value that failed to update
Comment 6 Coty Sutherland 2017-07-21 19:52:46 UTC
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