Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't allow referencing the pattern bank name in the pattern bank #29295

Conversation

@martijnvg
Copy link
Member

martijnvg commented Mar 29, 2018

Otherwise the grok code throws a stackoverflow error.

Closes #29257

libs/grok/src/test/java/org/elasticsearch/grok/GrokTests.java Outdated

e = expectThrows(IllegalArgumentException.class, () -> {
Map<String, String> bank = new HashMap<>();
bank.put("NAME", "!!!%{NAME:name}!!!");

This comment has been minimized.

Copy link
@talevy

talevy Mar 29, 2018

Contributor

if this is an attempt to catch more things... maybe add an example with type coercion as well?

bank.put("NAME", "!!!%{NAME:name:int}!!!");
@martijnvg

This comment has been minimized.

Copy link
Member Author

martijnvg commented Apr 3, 2018

@talevy I've added more tests and improved the error message.

libs/grok/src/main/java/org/elasticsearch/grok/Grok.java Outdated
if (pattern.contains("%{" + patternName + "}") || pattern.contains("%{" + patternName + ":")) {
String message;
if (path.isEmpty()) {
message = "circular reference in pattern bank [" + patternName + "][" + pattern + "]";

This comment has been minimized.

Copy link
@talevy

talevy Apr 3, 2018

Contributor

I feel like pattern bank is an internal naming convention, and externally we just call it patterns. not sure it matters here though, since one can only assume we are talking about the same thing.

libs/grok/src/main/java/org/elasticsearch/grok/Grok.java Outdated
byte[] expressionBytes = expression.getBytes(StandardCharsets.UTF_8);
this.compiledExpression = new Regex(expressionBytes, 0, expressionBytes.length, Option.DEFAULT, UTF8Encoding.INSTANCE);
}

private void forbidCircularReferences(String patternName, List<String> path, String pattern) {

This comment has been minimized.

Copy link
@talevy

talevy Apr 3, 2018

Contributor

we should probably add some javadocs here so when anyone picks this up in the future, they will have some context. it is very easy to get into the details of all the curly braces and lose track of why we are doing this.

libs/grok/src/main/java/org/elasticsearch/grok/Grok.java Outdated
} else if (brackedIndex != -1 && columnIndex != -1) {
end = Math.min(brackedIndex, columnIndex);
} else {
throw new IllegalArgumentException("cannot validate pattern [" + pattern + "] fo circular references");

This comment has been minimized.

Copy link
@talevy

talevy Apr 3, 2018

Contributor

s/fo/for?

maybe this can be simplified to just say pattern [<pattern>] has circular references to other pattern definitions?

libs/grok/src/main/java/org/elasticsearch/grok/Grok.java Outdated
"] back to pattern bank [" + patternName + "]";
// add rest of the path:
if (path.isEmpty() == false) {
message += " via pattern banks [" + String.join("=>", path) + "]";

This comment has been minimized.

Copy link
@talevy

talevy Apr 3, 2018

Contributor

nice! I like this. super helpful for keeping track

Copy link
Contributor

talevy left a comment

overall, this is sweet! I left some comments, let me know what you think. otherwise looks good to me

@martijnvg martijnvg force-pushed the martijnvg:ingest_fail_nicer_if_unable_to_process_grok_pattern branch Apr 4, 2018
@martijnvg

This comment has been minimized.

Copy link
Member Author

martijnvg commented Apr 4, 2018

@talevy Thanks, I've updated the PR.

@talevy
talevy approved these changes Apr 4, 2018
Copy link
Contributor

talevy left a comment

LGTM

…k processor.

Otherwise the grok code throws a stackoverflow error.

Closes #29257
@martijnvg martijnvg force-pushed the martijnvg:ingest_fail_nicer_if_unable_to_process_grok_pattern branch to 9da95ef Apr 5, 2018
@martijnvg martijnvg merged commit 9da95ef into elastic:master Apr 5, 2018
1 of 2 checks passed
1 of 2 checks passed
elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details
@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.