-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Disable regexes by default in painless #20427
Disable regexes by default in painless #20427
Conversation
@jdconrad, can you review this when you get a chance? |
@nik9000 LGTM! Thanks for tackling this issue. |
/** | ||
* Settings to use when compiling a script. | ||
*/ | ||
public final class CompilerSettings { | ||
/** | ||
* Are regexes enabled? This is a node level settings because regexes break out of painless's lovely sandbox and can cause stack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean 'setting' instead of 'settings' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to note, we do catch StackOverflows and consider that okay to happen within the sandbox. It won't bring down a node or anything like that. But the point does stand on not having the loop counter there. Maybe that's a crazy interesting project for regexes :) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll change to setting
.
I'd certainly love for regexes to be less brittle about this, yeah. Lucene's regexes are interesting because they have protection but they don't offer all the features we'd need. I'd love to take a crack at exposing them because they'd be safe but I'd like to get this in first.
Adds a new node level, non-dynamic setting, `script.painless.regex.enabled` can be used to enable regexes. Closes elastic#20397
Naive question: do you know off the top of your head what Lucene can't do that we want to provide right now? I think you, Robert, and I may have discussed this a while back, but my memory is poor. |
As it stands now I believe it doesn't support groups. Its implementation is very different than the one in the jvm. Instead of using backtracking it uses a DFA which it gets by building an NFA and doing a powerset conversion. This is super heavy up front but much easier to reason about. The interface it exposes would allow us to do a fairly comprehensive job locking it down with loop counters. Right now we rely on exposing the One of the funny things is that each regex dialect is going to be slightly different. It'll support different flags like case sensitivity and unicode normalization. Lucene is fairly minimal in that respect. They actual syntax is slightly different as well. We can't paper over any of these differences - we'd have to expose them to the user and make them deal with it. |
I mean, like, capturing groups. Back references might be coming I hear though, which is a good thing for groups. |
This still LGTM, so please feel to commit whenever you like. |
c2b2dcc
to
69bf08f
Compare
OK! All tests passed in master so I've pushed there. I'll backport to 5.1 and 5.0 branches. |
Adds a new node level, non-dynamic setting,
script.painless.regex.enabled
can be used to enable regexes.
Closes #20397