[
https://issues.apache.org/jira/browse/LOG4J2-3286?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17465049#comment-17465049
]
Carter Kozak commented on LOG4J2-3286:
--------------------------------------
> _Isn't catching Throwable considered a bad practice?_
I fear I provide this answer too frequently, but: It depends.
In most application code it's considered bad practice to catch and recover from
Error, however I wouldn't extend that to include infrastructure. I think the
answer is more obvious if you're writing a web-server: An Error should probably
result in a 500 response status, but not necessarily be allowed to terminate
the worker thread and terminate/leak the incoming connection. Similarly, when a
application code interacts with a logger, there's an expectation that it "just
works" (for some definition of works, it may or may not record data, but it
shouldn't throw!). Consider the following code block:
{code:java}
@Override
public void close() throws IOException {
log.debug("Closing {}", this);
releaseResources();
}{code}
I wouldn't want any sort of failure in log.debug to prevent
{{releaseResources()}} from being invoked!
The case is easier to make for some types of errors than others, but it's
easier to enumerate the types of errors we've seen in practice than those we
haven't.
Given log4j2 supports a wide range of plugins, some third-party plugins will
depend on other libraries that may be upgraded in ways that don't work at
runtime, throwing something like a NoClassDefFoundError or LinkageError. In
such a case, I'd still want the logging framework to continue processing my log
event, perhaps allowing it to be recorded by other appenders which aren't in a
broken state. If that's not what we're looking for, setting
{{ignore-exceptions="false"}} on the appender will allow exceptions to be
rethrown (although I expect they're caught again in AbstractLogger). You may
also consider using FailoverAppender with a synthetic appender (mocked?) and
use the interactions with the second appender to verify failures.
I broadened some of our catch blocks in 2.17.0 as a layer of defense in depth
against potential issues like CVE-2021-45105 – not as the primary protection,
we detect and avoid badness elsewhere, but if everything else goes wrong, the
logging framework should not be able to cause denial of service.
I've rambled long enough at this point, I hope my explanation was helpful :)
Let me know if you have further questions.
> 2.17 has reverted LOG4J-2972 fix in AppenderControl but change not in history
> -----------------------------------------------------------------------------
>
> Key: LOG4J2-3286
> URL: https://issues.apache.org/jira/browse/LOG4J2-3286
> Project: Log4j 2
> Issue Type: Question
> Components: Appenders
> Affects Versions: 2.17.0
> Environment: Windows desktop running unit tests with log4j 2.17
> Reporter: Ben Thurley
> Priority: Major
>
> Upgrading to 2.17 to fix the slew of vulnerabilities has resulted in a change
> to error handling in the logs.
> Specifically in the class:
> org.apache.logging.log4j.core.config.AppenderControl
> There is a method called tryCallAppender() which as of 2.14.1 had been set to
> catch Exception. Looking at the history I can see no further changes to
> revert this to handling Throwable, however, comparing the source for 2.16.
> and 2.17 this is exactly what has changed.
> I don't believe this change should have reverted and if it should then why
> isn't there a commit for it in the history?
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
