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?
+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.
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.
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.
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 :(
Created attachment 35123 [details] Patch proposal that logs the attribute name and value that failed to update
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