Bug 37182

Can you provide some sample-code?

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)

  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()?

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.

Created attachment 22346 [details]
null check before calling o.toString()

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.