Bug 37182 - Exception from exception toString() causes log4j to fail
Summary: Exception from exception toString() causes log4j to fail
Status: RESOLVED FIXED
Alias: None
Product: Log4j - Now in Jira
Classification: Unclassified
Component: Other (show other bugs)
Version: 1.2
Hardware: Other other
: P2 normal
Target Milestone: ---
Assignee: log4j-dev
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-10-20 14:47 UTC by Kari Ikonen
Modified: 2008-08-05 21:17 UTC (History)
2 users (show)



Attachments
null check before calling o.toString() (517 bytes, patch)
2008-08-02 10:21 UTC, Thorbjørn Ravn Andersen
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kari Ikonen 2005-10-20 14:47:07 UTC
If exception implements toString() -method and it fails (e.g. 
NullPointerException), it causes Log4J to throw exception back to calling 
application when such exception is tried to be traced.
Comment 1 Kay Abendroth 2006-11-11 13:36:42 UTC
Can you provide some sample-code?
Comment 2 Kari Ikonen 2006-11-28 10:42:09 UTC
Well, issue doesn't really need any specific test code, but....

--------------------------------
package org.kari.test;

import org.apache.log4j.BasicConfigurator;
import org.apache.log4j.Logger;

public class NullTest {
	private static final Logger LOG = Logger.getLogger("test");  

	public static class Null {
		private String mNull;
		@Override
		public String toString()
		{
			return mNull.toString();
		}
	}
	
	public static void main(String[] pArgs) {
        BasicConfigurator.configure();
		Null npe = new Null();
		try {
			LOG.fatal(npe);
		} catch (Exception e) {
			System.out.println("NPE OCCURRED IN LOG4J!");
			e.printStackTrace();
		}
	}
}
--------------------------------
OUTPUT:
---------------------
NPE OCCURRED IN LOG4J!
java.lang.NullPointerException
	at org.kari.test.NullTest$Null.toString(NullTest.java:14)
	at org.apache.log4j.or.DefaultRenderer.doRender(DefaultRenderer.java:26)
	at org.apache.log4j.or.RendererMap.findAndRender(RendererMap.java:70)
	at 
org.apache.log4j.spi.LoggingEvent.getRenderedMessage(LoggingEvent.java:288)
	at 
org.apache.log4j.helpers.PatternParser$BasicPatternConverter.convert(PatternParser.java:395)
	at 
org.apache.log4j.helpers.PatternConverter.format(PatternConverter.java:56)
	at org.apache.log4j.PatternLayout.format(PatternLayout.java:495)
	at org.apache.log4j.WriterAppender.subAppend(WriterAppender.java:292)
	at org.apache.log4j.WriterAppender.append(WriterAppender.java:150)
	at org.apache.log4j.AppenderSkeleton.doAppend(AppenderSkeleton.java:221)
	at 
org.apache.log4j.helpers.AppenderAttachableImpl.appendLoopOnAppenders(AppenderAttachableImpl.java:57)
	at org.apache.log4j.Category.callAppenders(Category.java:194)
	at org.apache.log4j.Category.forcedLog(Category.java:379)
	at org.apache.log4j.Category.fatal(Category.java:353)
	at org.kari.test.NullTest.main(NullTest.java:22)
Comment 3 Elias Ross 2007-01-26 20:02:27 UTC
  public void testToStringFail() {
    Object o = new Object() {
      public String toString() { throw new NullPointerException(); }
    };
    logger.debug(o);
  }


This currently fails with:

java.lang.NullPointerException
	at org.apache.log4j.CategoryTest$1.toString(CategoryTest.java:158)
	at org.apache.log4j.or.DefaultRenderer.doRender(DefaultRenderer.java:34)
	at org.apache.log4j.or.RendererMap.findAndRender(RendererMap.java:107)
	at org.apache.log4j.spi.LoggingEvent.getRenderedMessage(LoggingEvent.java:650)
	at
org.apache.log4j.pattern.MessagePatternConverter.format(MessagePatternConverter.java:58)
	at
org.apache.log4j.pattern.BridgePatternConverter.format(BridgePatternConverter.java:132)
	at org.apache.log4j.PatternLayout.format(PatternLayout.java:531)
	at org.apache.log4j.WriterAppender.subAppend(WriterAppender.java:360)
	at org.apache.log4j.WriterAppender.append(WriterAppender.java:175)
	at org.apache.log4j.AppenderSkeleton.doAppend(AppenderSkeleton.java:286)
	at
org.apache.log4j.helpers.AppenderAttachableImpl.appendLoopOnAppenders(AppenderAttachableImpl.java:67)
	at org.apache.log4j.Category.callAppenders(Category.java:218)
	at org.apache.log4j.Category.forcedLog(Category.java:588)
	at org.apache.log4j.Category.debug(Category.java:284)
	at org.apache.log4j.CategoryTest.testToStringFail(CategoryTest.java:160)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

What is the contract for Layout.format()?
Comment 4 Thorbjørn Ravn Andersen 2008-07-03 13:09:05 UTC
This is an issue with those classes which implement ObjectRenderer.  The DefaultRenderer (which is called here) just do a

  return o.toString();

and it would be reasonable to return some dummy value if o was null.  Just return "null" or null perhaps?

AttributesRenderer and ThreadGroupRenderer do an instanceof, UTObjectRenderer just returns a parmeters, so this should be the only location which need patching.
Comment 5 Thorbjørn Ravn Andersen 2008-08-02 10:21:34 UTC
Created attachment 22346 [details]
null check before calling o.toString()
Comment 6 Curt Arnold 2008-08-05 21:17:35 UTC
The first patch would be appropriate if the problem were:

logger.info(null);

However, that isn't the problem reported and control would not reach DefaultRenderer since LoggingEvent.getRenderedMessage would short circuit it.  

The problem is passing any object whose toString() method may result in either a RuntimeException or Error.  By far the most common message parameter is a String, but that is handled by a special case check in LoggingEvent.getRenderedMessage.  Adding the try/catch block in DefaultRenderer will add some overhead, but only in the uncommon case where the message isn't a String.  It isn't obvious what should be returned in that case, I have chosen to return the ex.toString() of the exception.  If you want something else, you could provide your own renderer (or make sure that your message object does not throw an exception).

Committed rev 683102.