You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/02/14 22:48:57 UTC
[GitHub] [logging-log4j2] ppkarwasz opened a new pull request #753: Issues with multiple Log4j 1.x filters
ppkarwasz opened a new pull request #753:
URL: https://github.com/apache/logging-log4j2/pull/753
This PR fixes a couple of issues concerning filters in the Log4j 1.x bridge:
- there is an endless loop in `FilterAdapter#filter` that hangs the program if more than one filter per appender is configured,
- filters are executed multiple times per log message.
The second problem comes from chains of Log4j 1.x filters: a filter chain is split into a `CompositeFilter` of `FilterAdapter`s (one for each filter). Each `FilterAdapter` executes its own filter and all those that come later in the chain.
In order to preserve the behavior of `FilterAdapter` the following restrictions have been applied to the generated filters:
1. no chains of Log4j 1.x filters are generated by the configuration factories; a list of Log4j 1.x filters is represented as a wrapped `CompositeFilter` of `FilterAdapter`s (or native Log4j 2.x filters),
2. the factories don't generate any `FilterWrapper`s of `FilterAdapter`s nor `FilterAdapter`s of `FilterWrapper`s.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on pull request #753: Issues with multiple Log4j 1.x filters
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on pull request #753:
URL: https://github.com/apache/logging-log4j2/pull/753#issuecomment-1045968986
Regarding the inclusion in version 2.17.2 (cf. [this thread](https://lists.apache.org/thread/fk538mcvyf6n8wsbxpfjrlnj9kk71hj8)), the two commented snippets of code are easy to verify and a must have.
The rest of the PR is only a performance optimization: in the worst case scenario of n chained legacy filters (each returning `NEUTRAL`), O(n^2) filters are executed per log message. In the more common scenario of n native Log4j 2.x filters, only n filters are evaluated (they "lose" the chain when the `AppenderWrapper` is unwrapped). In any case the evaluation of the filters gives always the correct result.
If a deadline is approaching, I wouldn't rush to include the whole PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #753: Issues with multiple Log4j 1.x filters
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #753:
URL: https://github.com/apache/logging-log4j2/pull/753#discussion_r806304150
##########
File path: log4j-1.2-api/src/main/java/org/apache/log4j/bridge/FilterAdapter.java
##########
@@ -37,14 +81,14 @@ public Result filter(LogEvent event) {
LoggingEvent loggingEvent = new LogEventAdapter(event);
Filter next = filter;
while (next != null) {
- switch (filter.decide(loggingEvent)) {
+ switch (next.decide(loggingEvent)) {
Review comment:
Infinite loop.
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/filter/CompositeFilter.java
##########
@@ -62,9 +62,9 @@ public CompositeFilter addFilter(final Filter filter) {
if (filter instanceof CompositeFilter) {
final int size = this.filters.length + ((CompositeFilter) filter).size();
final Filter[] copy = Arrays.copyOf(this.filters, size);
- final int index = this.filters.length;
+ int index = this.filters.length;
for (final Filter currentFilter : ((CompositeFilter) filter).filters) {
- copy[index] = currentFilter;
+ copy[index++] = currentFilter;
Review comment:
Small bug fix in `log4j-core`. Probably does not merit its own PR.
##########
File path: log4j-1.2-api/src/main/java/org/apache/log4j/bridge/FilterAdapter.java
##########
@@ -37,14 +81,14 @@ public Result filter(LogEvent event) {
LoggingEvent loggingEvent = new LogEventAdapter(event);
Filter next = filter;
while (next != null) {
- switch (filter.decide(loggingEvent)) {
+ switch (next.decide(loggingEvent)) {
Review comment:
Removes an infinite loop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #753: Issues with multiple Log4j 1.x filters
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #753:
URL: https://github.com/apache/logging-log4j2/pull/753#discussion_r806306691
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/filter/CompositeFilter.java
##########
@@ -62,9 +62,9 @@ public CompositeFilter addFilter(final Filter filter) {
if (filter instanceof CompositeFilter) {
final int size = this.filters.length + ((CompositeFilter) filter).size();
final Filter[] copy = Arrays.copyOf(this.filters, size);
- final int index = this.filters.length;
+ int index = this.filters.length;
for (final Filter currentFilter : ((CompositeFilter) filter).filters) {
- copy[index] = currentFilter;
+ copy[index++] = currentFilter;
Review comment:
Small bug fix in `log4j-core`. Probably does not merit its own PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #753: Issues with multiple Log4j 1.x filters
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #753:
URL: https://github.com/apache/logging-log4j2/pull/753#discussion_r806304150
##########
File path: log4j-1.2-api/src/main/java/org/apache/log4j/bridge/FilterAdapter.java
##########
@@ -37,14 +81,14 @@ public Result filter(LogEvent event) {
LoggingEvent loggingEvent = new LogEventAdapter(event);
Filter next = filter;
while (next != null) {
- switch (filter.decide(loggingEvent)) {
+ switch (next.decide(loggingEvent)) {
Review comment:
Infinite loop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #753: Issues with multiple Log4j 1.x filters
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #753:
URL: https://github.com/apache/logging-log4j2/pull/753#discussion_r806304150
##########
File path: log4j-1.2-api/src/main/java/org/apache/log4j/bridge/FilterAdapter.java
##########
@@ -37,14 +81,14 @@ public Result filter(LogEvent event) {
LoggingEvent loggingEvent = new LogEventAdapter(event);
Filter next = filter;
while (next != null) {
- switch (filter.decide(loggingEvent)) {
+ switch (next.decide(loggingEvent)) {
Review comment:
Removes an infinite loop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz edited a comment on pull request #753: Issues with multiple Log4j 1.x filters
Posted by GitBox <gi...@apache.org>.
ppkarwasz edited a comment on pull request #753:
URL: https://github.com/apache/logging-log4j2/pull/753#issuecomment-1045968986
Regarding the possible inclusion in version 2.17.2 (cf. [this thread](https://lists.apache.org/thread/fk538mcvyf6n8wsbxpfjrlnj9kk71hj8)), the two commented snippets of code are easy to verify and a must have.
The rest of the PR is only a performance optimization: in the worst case scenario of n chained legacy filters (each returning `NEUTRAL`), O(n^2) filters are executed per log message. In the more common scenario of n native Log4j 2.x filters, only n filters are evaluated (they "lose" the chain when the `AppenderWrapper` is unwrapped). In any case the evaluation of the filters gives always the correct result.
If a deadline is approaching, I wouldn't rush to include the whole PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory merged pull request #753: Issues with multiple Log4j 1.x filters
Posted by GitBox <gi...@apache.org>.
garydgregory merged pull request #753:
URL: https://github.com/apache/logging-log4j2/pull/753
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org