Bug 62090

Summary: NPE in o.a.t.util.modeler.Util when servlet-name does not exist in web.xml
Product: Tomcat 9 Reporter: Coty Sutherland <csutherl>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: normal Keywords: Beginner
Priority: P2    
Version: 9.0.4   
Target Milestone: -----   
Hardware: PC   
OS: Linux   

Description Coty Sutherland 2018-02-09 14:33:24 UTC
When using a web.xml that specifies a servlet element, but excludes servlet-name a NPE is observed and the webapp fails to deploy. A check should be added in the StandardWrapper and it should throw a ServletException when servlet-name is missing.

To reproduce, add a serlvet tag to your web.xml with no servlet-name. For example:

  <servlet>
    <servlet-class>com.example.servlets.HelloServlet</servlet-class>
  </servlet>

Then start Tomcat and observe the exception in catalina.out:

SEVERE [main] org.apache.catalina.core.ContainerBase.addChildInternal ContainerBase.addChild: start:
  org.apache.catalina.LifecycleException: Failed to initialize component [StandardEngine[Catalina].StandardHost[localhost].StandardContext[/test].StandardWrapper[null]]
    at org.apache.catalina.util.LifecycleBase.handleSubClassException(LifecycleBase.java:441)
    ....
    at org.apache.catalina.startup.Bootstrap.start(Bootstrap.java:353)
    at org.apache.catalina.startup.Bootstrap.main(Bootstrap.java:493)
Caused by: java.lang.NullPointerException
    at org.apache.tomcat.util.modeler.Util.objectNameValueNeedsQuote(Util.java:26)
    at org.apache.catalina.core.StandardWrapper.getObjectNameKeyProperties(StandardWrapper.java:1619)
    at org.apache.catalina.util.LifecycleMBeanBase.initInternal(LifecycleMBeanBase.java:61)
    at org.apache.catalina.core.ContainerBase.initInternal(ContainerBase.java:885)
    at org.apache.catalina.util.LifecycleBase.init(LifecycleBase.java:136)
    ... 49 more

Setting xmlValidation="true" on the Context may handle this differently, but it's off by default so we should check for null and handle that case.
Comment 1 Christopher Schultz 2018-02-09 16:17:27 UTC
The XML schema *requires the servlet-name element. (Note that the servlet-class element is not required.) Tomcat simply doesn't use the schema to validate web.xml, so this "error" falls through the cracks.

I understand why we don't validate context.xml and server.xml against a schema... but why not enable validation of web.xml against the correct schema?
Comment 2 Remy Maucherat 2018-02-09 16:21:47 UTC
XML validation is annoying IMO, having checks for functionally significant items can be useful (also, the issue here might exist with embedded or some other use maybe ?).
Comment 3 Christopher Schultz 2018-02-09 16:50:42 UTC
How is it annoying?

Just set "setSchema(schema); setValidating(false);".

It's only going to validate once per context-launch, so this isn't a performance problem.

Validating XML documents against their respective schemas is a good way to ensure that everything is sane before Tomcat tries to use the data. It allows the Tomcat code to rely on a sane document before proceeding.

Otherwise, you get NPEs and other stupid things like that.
Comment 4 Slava Risenberg 2018-02-20 12:34:22 UTC
There are two issues to consider:
1. Don't think that enabling XML validation rapidly against DTD is a good idea, Tomcat has a large user base and I assume that a lot of users have broken/invalid web.xml file that would fail the validation.
2. It is possible to add manual validation, as originally suggested by Coty Sutherland, handle it nicely instead of throwing NPE, and print meaningful error in log.  

As this bug/issue is marked as beginner-friendly I could do that, what do you think?
Comment 5 Slava Risenberg 2018-02-20 15:38:06 UTC
(In reply to Coty Sutherland from comment #0)
> When using a web.xml that specifies a servlet element, but excludes
> servlet-name a NPE is observed and the webapp fails to deploy. A check
> should be added in the StandardWrapper and it should throw a
> ServletException when servlet-name is missing.
> 
> To reproduce, add a serlvet tag to your web.xml with no servlet-name. For
> example:
> 
>   <servlet>
>     <servlet-class>com.example.servlets.HelloServlet</servlet-class>
>   </servlet>
> 
> Then start Tomcat and observe the exception in catalina.out:
> 
> SEVERE [main] org.apache.catalina.core.ContainerBase.addChildInternal
> ContainerBase.addChild: start:
>   org.apache.catalina.LifecycleException: Failed to initialize component
> [StandardEngine[Catalina].StandardHost[localhost].StandardContext[/test].
> StandardWrapper[null]]
>     at
> org.apache.catalina.util.LifecycleBase.handleSubClassException(LifecycleBase.
> java:441)
>     ....
>     at org.apache.catalina.startup.Bootstrap.start(Bootstrap.java:353)
>     at org.apache.catalina.startup.Bootstrap.main(Bootstrap.java:493)
> Caused by: java.lang.NullPointerException
>     at
> org.apache.tomcat.util.modeler.Util.objectNameValueNeedsQuote(Util.java:26)
>     at
> org.apache.catalina.core.StandardWrapper.
> getObjectNameKeyProperties(StandardWrapper.java:1619)
>     at
> org.apache.catalina.util.LifecycleMBeanBase.initInternal(LifecycleMBeanBase.
> java:61)
>     at
> org.apache.catalina.core.ContainerBase.initInternal(ContainerBase.java:885)
>     at org.apache.catalina.util.LifecycleBase.init(LifecycleBase.java:136)
>     ... 49 more
> 
> Setting xmlValidation="true" on the Context may handle this differently, but
> it's off by default so we should check for null and handle that case.

A check should be added to ContextConfig.configureContext() method as it is the place where context is being populated with servlets properties.
Comment 6 Remy Maucherat 2018-02-20 16:02:14 UTC
I'd rather add an IAE to ContainerBase.setName.
Comment 7 Remy Maucherat 2018-02-20 16:18:47 UTC
The fix will be in 9.0.6, 8.5.29, 8.0.51 and 7.0.86.
Comment 8 Slava Risenberg 2018-02-21 09:41:26 UTC
(In reply to Remy Maucherat from comment #6)
> I'd rather add an IAE to ContainerBase.setName.

ContainerBase is an abstract class, making change in it will affect other irrelevant Classes like StandardContext, StandardEngine, StandardHost and StandardWrapper, so it's definitely not a good place for the fix.
Comment 9 Remy Maucherat 2018-02-21 13:32:31 UTC
For starters, wrapper = servlet so that should be ok. Then containers are supposed to have names in Catalina (they go in children maps keyed by name ...), so it's good to do it that way.
Comment 10 Christopher Schultz 2018-02-21 15:08:26 UTC
(In reply to Slava Risenberg from comment #8)
> (In reply to Remy Maucherat from comment #6)
> > I'd rather add an IAE to ContainerBase.setName.
> 
> ContainerBase is an abstract class, making change in it will affect other
> irrelevant Classes like StandardContext, StandardEngine, StandardHost and
> StandardWrapper, so it's definitely not a good place for the fix.

Also, the default value for "name" is null, and this check can only be made it setName(String) is actually called. Assuming Tomcat uses digester to parse XML files, a missing servlet-name will skip the call to ContainerBase.setName(String), no?