Skip to content

Commit

Permalink
Catching StackOverflowErrors from bad regexes in GsubProcessor and re…
Browse files Browse the repository at this point in the history
…throwing as an Exception (elastic#106851)
  • Loading branch information
masseyke authored Apr 11, 2024
1 parent 6ff3a26 commit ef16be9
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 1 deletion.
5 changes: 5 additions & 0 deletions docs/changelog/106851.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 106851
summary: Catching `StackOverflowErrors` from bad regexes in `GsubProcessor`
area: Ingest Node
type: bug
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@

package org.elasticsearch.ingest.common;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import java.util.Map;
import java.util.regex.Pattern;

Expand All @@ -21,6 +24,7 @@
public final class GsubProcessor extends AbstractStringProcessor<String> {

public static final String TYPE = "gsub";
private static final Logger logger = LogManager.getLogger(GsubProcessor.class);

private final Pattern pattern;
private final String replacement;
Expand Down Expand Up @@ -49,7 +53,19 @@ String getReplacement() {

@Override
protected String process(String value) {
return pattern.matcher(value).replaceAll(replacement);
try {
return pattern.matcher(value).replaceAll(replacement);
} catch (StackOverflowError e) {
/*
* A bad regex on problematic data can trigger a StackOverflowError. In this case we can safely recover from the
* StackOverflowError, so we rethrow it as an Exception instead. This way the document fails this processor, but processing
* can carry on. The value would be useful to log here, but we do not do so for because we do not want to write potentially
* sensitive data to the logs.
*/
String message = "Caught a StackOverflowError while processing gsub pattern: [" + pattern + "]";
logger.trace(message, e);
throw new IllegalArgumentException(message);
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@

package org.elasticsearch.ingest.common;

import org.elasticsearch.ingest.IngestDocument;
import org.elasticsearch.ingest.RandomDocumentPicks;

import java.util.Map;
import java.util.regex.Pattern;

public class GsubProcessorTests extends AbstractStringProcessorTestCase<String> {
Expand All @@ -26,4 +30,23 @@ protected String modifyInput(String input) {
protected String expectedResult(String input) {
return "127-0-0-1";
}

public void testStackOverflow() {
// This tests that we rethrow StackOverflowErrors as ElasticsearchExceptions so that we don't take down the node
String badRegex = "( (?=(?:[^'\"]|'[^']*'|\"[^\"]*\")*$))";
GsubProcessor processor = new GsubProcessor(
randomAlphaOfLength(10),
null,
"field",
Pattern.compile(badRegex),
"-",
false,
"targetField"
);
StringBuilder badSourceBuilder = new StringBuilder("key1=x key2=");
badSourceBuilder.append("x".repeat(3000));
Map<String, Object> source = Map.of("field", badSourceBuilder.toString());
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), source);
IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> processor.execute(ingestDocument));
}
}

0 comments on commit ef16be9

Please sign in to comment.