https://issues.apache.org/bugzilla/show_bug.cgi?id=56166
Bug ID: 56166
Summary: Suggestions for exception handling (avoid potential
bugs)
Product: Tomcat 8
Version: trunk
Hardware: All
OS: All
Status: NEW
Severity: normal
Priority: P2
Component: Catalina
Assignee: dev@tomcat.apache.org
Reporter: y...@eecg.utoronto.ca
Created attachment 31337
--> https://issues.apache.org/bugzilla/attachment.cgi?id=31337&action=edit
Patch addressing the mentioned problems.
Reporting a few potential bugs or improvements on some exception handling code.
For what it is worth, attaching a patch to address these problems.
Category 1:
The exception caught is too general:
=========================
Case 1:
Line: 180, File: "org/apache/catalina/core/ApplicationFilterFactory.java"
178: try {
179: isCometFilter = filterConfig.getFilter() instanceof CometFilter;
180: } catch (Exception e) {
181: // Note: The try catch is there because getFilter has a lot of
182: // declared exceptions. However, the filter is allocated much
183: // earlier
184: }
This try block can throw many exceptions, including:
1. ClassCastException
2. ClassNotFoundException
3. IllegalAccessException
4. InstantiationException
5. ServletException
6. NamingException
7. InvocationTargetException
All of them will be silently swallowed by the catch block. In particular,
earlier in the same function, the same exception is handled in the following
manner:
try {
isCometFilter = filterConfig.getFilter() instanceof CometFilter;
} catch (Exception e) {
// Note: The try catch is there because getFilter has a lot of
// declared exceptions. However, the filter is allocated much
// earlier
Throwable t = ExceptionUtils.unwrapInvocationTargetException(e);
ExceptionUtils.handleThrowable(t);
}
Fixed in the patch by handling the exception in the same way as the above.
==========================================
=========================
Case 2:
Line: 234, File: "org/apache/tomcat/util/modeler/BaseModelMBean.java"
232: try {
233: response.add(new Attribute(names[i],getAttribute(names[i])));
234: } catch (Exception e) {
235: // Not having a particular attribute in the response
236: // is the indication of a getter problem
237: }
The try block could throw:
1. AttributeNotFoundException
2. MBeanException
3. ReflectionException
Fixed by ignoring the AttributeNotFoundException and log the other exceptions.
==========================================
=========================
Case 3:
Line: 298, File: "org/apache/catalina/manager/StatusTransformer.java"
294: try {
295: Object value = mBeanServer.getAttribute(tpName, "keepAliveCount");
296: writer.print(" Keeped alive sockets count: ");
297: writer.print(value);
298: } catch (Exception e) {
299: // Ignore
300: }
This might be problematic since the catch block is too general.
getAttribute() can throw:
1. RuntimeOperationsException
2. MBeanException
3. AttributeNotFoundException
4. InstanceNotFoundException
5. ReflectionException
All of them will be swallowed.
Fixed by only ignoring AttributeNotFoundException.
==========================================
=========================
Case 4:
Line: 215, File: "org/apache/catalina/storeconfig/StoreLoader.java"
206: try {
207: File home = new File(getCatalinaBase());
208: File conf = new File(home, "conf");
209: File reg = new File(conf, "server-registry.xml");
210: is = new FileInputStream(reg);
211: if (log.isInfoEnabled())
212: log.info("Find registry server-registry.xml at file "
213: + reg.getCanonicalPath());
214: registryResource = reg.toURI().toURL();
215: } catch (Throwable t) {
216: // Ignore
217: }
The catch block might be too general.
The try block could throw:
1. NullPointerException by new File
2. FileNotFoundException by new FileInputStream
3. SecurityException by new FileInputStream and reg.toURL
4. IllegalArgumentException by reg.toURL().toURL
5. MalformedURLException by reg.toURL().toURL
Now with the implementation above, all of them will be ignored.
Fixed by ignoring FileNotFoundException while logging the others.
==========================================
=========================
Case 5:
Line: 307, File: "org/apache/tomcat/util/modeler/ManagedBean.java"
305: try {
306: clazz = Class.forName(getClassName());
307: } catch (Exception e) {
308: }
Should probably just ignore ClassNotFoundException.
Fixed by ignoring ClassNotFoundException.
==========================================
=========================
Case 6:
Line: 202, File: "org/apache/catalina/storeconfig/StoreLoader.java"
193: try {
194: String configUrl = getConfigUrl();
195: if (configUrl != null) {
196: is = (new URL(configUrl)).openStream();
197: if (log.isInfoEnabled())
198: log.info("Find registry server-registry.xml from system
property at url "
199: + configUrl);
200: registryResource = new URL(configUrl);
201: }
202: } catch (Throwable t) {
203: // Ignore
204: }
The catch block might be too general.
openStream() above could throw IOException;
new URL() could throw MalformedURLException;
Both will be ignored.
Fixed by logging MalformedURLException and ignoring IOException.
==========================================
Category 2: Exceptions that "should never happen" are now silently ignored. In
those cases, it is better to at least put an error message there, if not
shutting down the execution. Later as code evolves, or something really wrong
occurs, such exception could have occurred and would be silently swallowed.
Currently I fixed them by log these exceptions.
=========================
Case 7:
Line: 867, File: "org/apache/catalina/connector/CoyoteAdapter.java"
865: try {
866: Thread.sleep(1000);
867: } catch (InterruptedException e) {
868: // Should never happen
869: }
==========================================
=========================
Case 8:
Line: 706, File: "org/apache/catalina/core/DefaultInstanceManager.java"
703: try {
704: result = clazz.getDeclaredField(
705: entry.getAccessibleObjectName());
706: } catch (NoSuchFieldException e) {
707: // Should never happen. On that basis don't log
708: // it.
709: }
==========================================
=========================
Case 9:
Line: 231, File: "org/apache/catalina/core/NamingContextListener.java"
229: try {
230: namingContext = new NamingContext(contextEnv, getName());
231: } catch (NamingException e) {
232: // Never happens
233: }
==========================================
=========================
Case 10:
Line: 465, File: "org/apache/catalina/connector/Request.java"
463: try {
464: part.delete();
465: } catch (IOException ignored) {
466: // ApplicationPart.delete() never throws an IOEx
467: }
==========================================
=========================
Case 11:
Line: 2333, File: "org/apache/catalina/connector/Request.java"
2331: try {
2332: logout();
2333: } catch (ServletException e) {
2334: // Should never happen (no code called by logout()
2335: // throws a ServletException
2336: }
==========================================
=========================
Case 12:
Line: 481, File: "org/apache/catalina/valves/rewrite/RewriteValve.java"
478: try {
479:
request.getConnector().getProtocolHandler().getAdapter().service
480: (request.getCoyoteRequest(), response.getCoyoteResponse());
481: } catch (Exception e) {
482: // This doesn't actually happen in the Catalina adapter
implementation
483: }
==========================================
=========================
Case 13:
Line: 844, File: "org/apache/tomcat/jdbc/pool/PoolProperties.java"
816:try {
817: String[] fields = DataSourceFactory.ALL_PROPERTIES;
818: for (String field: fields) {
818: for (String field: fields) {
819: final String[] prefix = new String[] {"get","is"};
820: for (int j=0; j<prefix.length; j++) {
821:
822: String name = prefix[j]
823: + field.substring(0, 1).toUpperCase(Locale.ENGLISH)
824: + field.substring(1);
825: Method m = null;
826: try {
827: m = getClass().getMethod(name);
828: }catch (NoSuchMethodException nm) {
829: continue;
830: }
831: buf.append(field);
832: buf.append("=");
833: if (DataSourceFactory.PROP_PASSWORD.equals(field)) {
834: buf.append("********");
835: } else {
836: buf.append(m.invoke(this, new Object[0]));
837: }
838: buf.append("; ");
839: break;
840: }
841: }
842:}catch (Exception x) {
843: //shouldn't happen
844: log.debug("toString() call failed", x);
845:}
Fixed by changing verbosity from debug to error.
==========================================
Exceptions that probably should at least be logged:
=========================
Case 14:
Line: 480, File: "org/apache/juli/ClassLoaderLogManager.java"
478: try {
479: is = new FileInputStream(defaultFile);
480: } catch (IOException e) {
481: // Critical problem, do something ...
482: }
Fixed by logging it.
==========================================
=========================
Case 15:
Line: 276, File: "org/apache/catalina/authenticator/SpnegoAuthenticator.java"
274: try {
275: lc.logout();
276: } catch (LoginException e) {
277: // Ignore
278: }
This LoginException during logout should not be ignored. Fixed by logging it.
==========================================
=========================
Case 16:
Line: 269, File: "org/apache/catalina/authenticator/SpnegoAuthenticator.java"
267: try {
268: gssContext.dispose();
269: } catch (GSSException e) {
270: // Ignore
271: }
Fixed by logging it.
==========================================
=========================
Case 17:
Line: 348, File: "org/apache/catalina/core/StandardServer.java"
346: try {
347: service.start();
348: } catch (LifecycleException e) {
349: // Ignore
350: }
Similar code snippets are logged (LifecycleException thrown from start()):
Such as:
try {
connector.start();
} catch (LifecycleException e) {
log.error(sm.getString(
"standardService.connector.startFailed",
connector), e);
}
try {
ex.start();
} catch (LifecycleException x) {
log.error("Executor.start", x);
}
Fixed by logging it.
==========================================
=========================
Case 18:
Line: 150, File: "org/apache/catalina/core/StandardService.java"
148: try {
149: this.container.start();
150: } catch (LifecycleException e) {
151: // Ignore
152: }
Fixed by logging it. similar to the case above.
==========================================
=========================
Case 19:
Line 141, File "org/apache/catalina/core/ApplicationFilterFactory.java"
if (filterConfig == null) {
// FIXME - log configuration problem
continue;
}
Fixed by logging it.
A similar case is at line 173 in the same file.
==========================================
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
