StringMatchFilter would return NPE if configured programmatically with 'null' text.

[JWT007]

StringMatchFilter (Log4j 2.24.1)

When parsing from XML I think if the "text" attribute is missing it will be populated with an empty string due to the Builder field:

@PluginBuilderAttribute
private String text = "";

However, if for whatever reason, someone was programmatically creating this StringMatchFilter and passed null to "Builder#setMatchString", no validation is performed in the "build()" method or the constructor.

This would lead to a deferred NPE in the StringMatchFilter#filter method:

private Result filter(final String msg) {
   return msg.contains(this.text) ? onMatch : onMismatch;
}

Since String#contains(s) assumes s is NotNull.

public boolean contains(CharSequence s) {
   return indexOf(s.toString()) >= 0;  // <== NPE!
}

I thiink standard behaviour would be to log an error and return null in the 'build()' method if the 'text' field is null.

Also there seems to be a copy/paste error in the StringMatchFilter.Builder#setMarkerText javadoc:

/**
 * Sets the logging level to use.
 * @param text the logging level to use
 * @return this
 */
public StringMatchFilter.Builder setMatchString(final String text) {
    this.text = text;
    return this;
}

[ppkarwasz]

@JWT007,

Thank you for catching this problem. I believe that the missing validation affects both types of configuration.

When configuring through a configuration file, it does not make sense to have an empty text configuration property. The field should be annotated with @Required, which in this case will actually check that the parameter is not empty.

In case of programmatic configuration there are two possible solutions:

1. We can use the Builder.isValid() method I introduced in Improves validation of Log4j 2.x components #771, e.g. if (!isValid()) return false;. This has the advantage of working even if the caller forgets to call setMatchString on the builder.
2. We can add a call to Assert.requireNonEmpty() in the setter. I was not a big fan of this solution in Improve validation of PathConditions #1231, but it doesn't seem so bad to me right now.

Probably we should implement both solutions at once. Would you be willing to make a PR?

[JWT007]

Hi @ppkarwasz thanks for your feedback.

Unfortunately I am a bit too buried in my day job to be actively contributing - but since I am pouring over the Log4j code, I am at least trying to call a few things out where I see them. :)

[JWT007]

@ppkarwasz - I take it back ... I was curious if I could manage the PR since I don't do Github much.

I created a PR but first time so please double-check that everything is there.

[JWT007]

@ppkarwasz - one more question :) how do relevant fixes find their way into 3.x?

[ppkarwasz]

@ppkarwasz - one more question :) how do relevant fixes find their way into 3.x?

That is an excellent question. We try to port them immediately, but there is usually a backlog. I have written down part of the backlog in #3161.

[JWT007]

@ppkarwasz - OK is that something I would need to do (i.e. PR against master) or something you do internally.

[ppkarwasz]

It's optional: you can create a PR and we pretty much appreciate it, but we can also push it directly to main.
In this case I think that the second option is faster (I don't expect any conflicts).

[JWT007]

@ppkarwasz - seems the PR for this one was merged too. Close as done?

[JWT007]

This 2.x fix is implemented with #3509 in main (3.x)