Scripting: enable regular expressions by default #63029

stu-elastic · 2020-09-29T17:11:06Z

Setting script.painless.regex.enabled has a new option,
limited, the default. This defaults to using regular
expressions but limiting the complexity of the regular
expressions.

In addition to limited, the setting can be true, as
before, which enables regular expressions without limiting them.

false totally disables regular expressions, which was the
old default.
New setting script.painless.regex.limit-factor. This limits
regular expression complexity by limiting the number characters
a regular expression can consider based on input length.

The default is 6, so a regular expression can consider
6 * input length number of characters. With input
foobarbaz (length 9), for example, the regular expression
can consider 54 (6 * 9) characters.

This reduces the impact of exponential backtracking in Java's
regular expression engine.
add @inject_constant annotation to whitelist.

This annotation signals that a compiler settings will
be injected at the beginning of a whitelisted method.

The format is argnum=settingname:
1=foo_setting 2=bar_setting.

Argument numbers must start at one and must be sequential.
Augment
Pattern.split(CharSequence)
Pattern.split(CharSequence, int),
Pattern.splitAsStream(CharSequence)
Pattern.matcher(CharSequence)
to take the value of script.painless.regex.limit-factor as a
an injected parameter, limiting as explained above when this
setting is in use.

* Setting `script.painless.regex.enabled` has a new option, `use-factor`, the default. This defaults to using regular expressions but limiting the complexity of the regular expressions. In addition to `use-factor`, the setting can be `true`, as before, which enables regular expressions without limiting them. `false` totally disables regular expressions, which was the old default. * New setting `script.painless.regex.limit-factor`. This limits regular expression complexity by limiting the number characters a regular expression can consider based on input length. The default is `6`, so a regular expression can consider `6` * input length number of characters. With input `foobarbaz` (length `9`), for example, the regular expression can consider `54` (`6 * 9`) characters. This reduces the impact of exponential backtracking in Java's regular expression engine. * add `@inject_constant` annotation to whitelist. This annotation signals that a compiler settings will be injected at the beginning of a whitelisted method. The format is `argnum=settingname`: `1=foo_setting 2=bar_setting`. Argument numbers must start at one and must be sequential. * Augment `Pattern.split(CharSequence)` `Pattern.split(CharSequence, int)`, `Pattern.splitAsStream(CharSequence)` `Pattern.matcher(CharSequence)` to take the value of `script.painless.regex.limit-factor` as a an injected parameter, limiting as explained above when this setting is in use. Fixes: elastic#49873

rjernst

I just have a few high level comments mostly. First is on the naming. "use-factor" is pretty cryptic. Can we use something like "limited"? This flows nicely to me. If I ask the question "are regexes enabled?" the answer true/false/limited. Second, it seems there are quite a few TODOs leftover that can probably be removed? If they are to be left in then please expand on them and/or create an issue so someone else on the team could understand what needs to be done.

rjernst · 2020-09-29T17:24:11Z

modules/lang-painless/src/main/resources/org/elasticsearch/painless/spi/java.util.regex.txt

@@ -58,6 +58,7 @@ class java.util.regex.Matcher {
  String replaceFirst(String)
  boolean requireEnd()
  Matcher reset()
+  # Whitelisting Matcher.reset(String) works around the regex limiting


Since we don't actually whitelist this method, I think this is just a comment to note if we did allow that method it would allow escaping the regex limiting? Could you clarify the comment?

Updated comment.

rjernst · 2020-09-29T17:24:24Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/symbol/ScriptScope.java

@@ -66,6 +66,8 @@ public ScriptScope(PainlessLookup painlessLookup, CompilerSettings compilerSetti
        staticConstants.put("$SOURCE", scriptSource);
        staticConstants.put("$DEFINITION", painlessLookup);
        staticConstants.put("$FUNCTIONS", functionTable);
+        // TODO(stu): inject compiler settings here


leftover todo?

Yup, these and all the left over todos were why this was initially a draft. They are gone now.

rjernst · 2020-09-29T17:33:21Z

...g-painless/src/main/java/org/elasticsearch/painless/phase/PainlessUserTreeToIRTreePhase.java

@@ -211,7 +211,7 @@ protected void injectStaticFieldsAndGetters() {
        irLoadFieldMemberNode.setLocation(internalLocation);
        irLoadFieldMemberNode.setExpressionType(String.class);
        irLoadFieldMemberNode.setName("$NAME");
-        irLoadFieldMemberNode.setStatic(true);
+        irLoadFieldMemberNode.setStatic(true); // TODO(stu): add $COMPILER_INJECTS, add hash map and set it


what is this todo?

rjernst · 2020-09-29T17:35:50Z

.../src/main/java/org/elasticsearch/painless/spi/annotation/InjectConstantAnnotationParser.java

+            if (arguments.containsKey(argNum) == false) {
+                throw new IllegalArgumentException("[@inject_constant] missing argument number [" + argNum + "]");
+            }
+            // TODO(stu): Jack, how do I verify against CompilerSettings.


Todos like this are very cryptic. Can we just have a normal comment if some explanation is needed?

Removed when draft.

rjernst · 2020-09-29T17:36:33Z

...ss/spi/src/main/java/org/elasticsearch/painless/spi/annotation/InjectConstantAnnotation.java

+import java.util.Collections;
+import java.util.List;
+
+public class InjectConstantAnnotation {


Can we have some basic java docs on this?

elasticmachine · 2020-09-30T14:49:51Z

Pinging @elastic/es-core-infra (:Core/Infra/Scripting)

jdconrad · 2020-09-30T18:23:28Z

@stu-elastic One note is we need to ensure we support the operators ==~ and =~ in the BinaryMathNode.write method. Apologies as I dropped the ball on this one.

rjernst

LGTM

* Setting `script.painless.regex.enabled` has a new option, `use-factor`, the default. This defaults to using regular expressions but limiting the complexity of the regular expressions. In addition to `use-factor`, the setting can be `true`, as before, which enables regular expressions without limiting them. `false` totally disables regular expressions, which was the old default. * New setting `script.painless.regex.limit-factor`. This limits regular expression complexity by limiting the number characters a regular expression can consider based on input length. The default is `6`, so a regular expression can consider `6` * input length number of characters. With input `foobarbaz` (length `9`), for example, the regular expression can consider `54` (`6 * 9`) characters. This reduces the impact of exponential backtracking in Java's regular expression engine. * add `@inject_constant` annotation to whitelist. This annotation signals that a compiler settings will be injected at the beginning of a whitelisted method. The format is `argnum=settingname`: `1=foo_setting 2=bar_setting`. Argument numbers must start at one and must be sequential. * Augment `Pattern.split(CharSequence)` `Pattern.split(CharSequence, int)`, `Pattern.splitAsStream(CharSequence)` `Pattern.matcher(CharSequence)` to take the value of `script.painless.regex.limit-factor` as a an injected parameter, limiting as explained above when this setting is in use. Fixes: elastic#49873

* Setting `script.painless.regex.enabled` has a new option, `use-factor`, the default. This defaults to using regular expressions but limiting the complexity of the regular expressions. In addition to `use-factor`, the setting can be `true`, as before, which enables regular expressions without limiting them. `false` totally disables regular expressions, which was the old default. * New setting `script.painless.regex.limit-factor`. This limits regular expression complexity by limiting the number characters a regular expression can consider based on input length. The default is `6`, so a regular expression can consider `6` * input length number of characters. With input `foobarbaz` (length `9`), for example, the regular expression can consider `54` (`6 * 9`) characters. This reduces the impact of exponential backtracking in Java's regular expression engine. * add `@inject_constant` annotation to whitelist. This annotation signals that a compiler settings will be injected at the beginning of a whitelisted method. The format is `argnum=settingname`: `1=foo_setting 2=bar_setting`. Argument numbers must start at one and must be sequential. * Augment `Pattern.split(CharSequence)` `Pattern.split(CharSequence, int)`, `Pattern.splitAsStream(CharSequence)` `Pattern.matcher(CharSequence)` to take the value of `script.painless.regex.limit-factor` as a an injected parameter, limiting as explained above when this setting is in use. Fixes: #49873 Backport of: 93f29a4

stu-elastic · 2020-10-05T18:18:13Z

master: 93f29a4
7.x (7.10): 791a9d5

Now that we've got regexes enabled by default (elastic#63029) this adds a test to runtime fields just to make sure that it works with regexes. It does, but this adds a test to make sure it continues to work.

Now that we've got regexes enabled by default (#63029) this adds a test to runtime fields just to make sure that it works with regexes. It does, but this adds a test to make sure it continues to work.

Now that we've got regexes enabled by default (elastic#63029) this adds a test to runtime fields just to make sure that it works with regexes. It does, but this adds a test to make sure it continues to work.

Now that we've got regexes enabled by default (#63029) this adds a test to runtime fields just to make sure that it works with regexes. It does, but this adds a test to make sure it continues to work.

Now that we've got regexes enabled by default (elastic#63029) this adds a test to runtime fields just to make sure that it works with regexes. It does, but this adds a test to make sure it continues to work.

Documents the `script.painless.regex.enabled` and `script.painless.regex.limit-factor` cluster settings. Relates to #63029. Closes #75199.

Documents the `script.painless.regex.enabled` and `script.painless.regex.limit-factor` cluster settings. Relates to elastic#63029. Closes elastic#75199.

Documents the `script.painless.regex.enabled` and `script.painless.regex.limit-factor` cluster settings. Relates to #63029. Closes #75199. Co-authored-by: James Rodewig <40268737+jrodewig@users.noreply.github.com>

Documents the `script.painless.regex.enabled` and `script.painless.regex.limit-factor` cluster settings. Relates to #63029. Closes #75199.

stu-elastic added >enhancement :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v8.0.0 v7.10.0 labels Sep 29, 2020

stu-elastic requested a review from rjernst September 29, 2020 17:11

Add javadoc, remove done TODOS

6d58686

rjernst requested changes Sep 29, 2020

View reviewed changes

Regexes are enabled by default

3a139fc

stu-elastic marked this pull request as ready for review September 30, 2020 14:49

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Sep 30, 2020

use_factor -> limited, InjectConstantAnnotation javadocs

b89bea1

stu-elastic added 10 commits October 1, 2020 11:39

merge

3895974

find and match operator

8e17ec8

Set limit factor for Find, add tests

51bda05

Add pattern

a8c838d

Add tests for unlimited

f6e0363

fix style problems

8216391

Skip todo

e8ebf4d

add snippet

6a2ed98

unused import

60bc11a

No longer disabled

297d440

rjernst approved these changes Oct 2, 2020

View reviewed changes

stu-elastic merged commit 93f29a4 into elastic:master Oct 5, 2020

nik9000 mentioned this pull request Oct 20, 2020

Add a test for regex usage to runtime fields #63951

Merged

nik9000 mentioned this pull request Oct 20, 2020

Add a test for regex usage to runtime fields (backport #63951) #63960

Merged

javanna mentioned this pull request Nov 25, 2020

Update docs on using regex in painless #65496

Closed

axw mentioned this pull request Dec 3, 2020

Investigate moving sourcemapping to an enrich processor elastic/apm-server#3606

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

jrodewig mentioned this pull request Aug 3, 2021

[DOCS] Document regex circuit breaker #76048

Merged

jrodewig added a commit that referenced this pull request Aug 4, 2021

[DOCS] Document regex circuit breaker (#76048)

5252995

Documents the `script.painless.regex.enabled` and `script.painless.regex.limit-factor` cluster settings. Relates to #63029. Closes #75199.

jrodewig added a commit that referenced this pull request Aug 4, 2021

[DOCS] Document regex circuit breaker (#76048) (#76132)

cafc5e0

Documents the `script.painless.regex.enabled` and `script.painless.regex.limit-factor` cluster settings. Relates to #63029. Closes #75199.

rjernst mentioned this pull request Apr 2, 2024

Catching StackOverflowErrors from bad regexes in GsubProcessor #106851

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scripting: enable regular expressions by default #63029

Scripting: enable regular expressions by default #63029

stu-elastic commented Sep 29, 2020 •

edited

Loading

rjernst left a comment

rjernst Sep 29, 2020

stu-elastic Sep 30, 2020

stu-elastic Sep 30, 2020

rjernst Sep 29, 2020

stu-elastic Sep 30, 2020

rjernst Sep 29, 2020

stu-elastic Sep 30, 2020

rjernst Sep 29, 2020

stu-elastic Sep 30, 2020

rjernst Sep 29, 2020

stu-elastic Sep 30, 2020

stu-elastic Sep 30, 2020

elasticmachine commented Sep 30, 2020

jdconrad commented Sep 30, 2020

rjernst left a comment

stu-elastic commented Oct 5, 2020

Scripting: enable regular expressions by default #63029

Scripting: enable regular expressions by default #63029

Conversation

stu-elastic commented Sep 29, 2020 • edited Loading

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 30, 2020

jdconrad commented Sep 30, 2020

rjernst left a comment

Choose a reason for hiding this comment

stu-elastic commented Oct 5, 2020

stu-elastic commented Sep 29, 2020 •

edited

Loading