Stop runtime script from emitting too many values#61938
Merged
nik9000 merged 5 commits intoelastic:masterfrom Sep 9, 2020
Merged
Stop runtime script from emitting too many values#61938nik9000 merged 5 commits intoelastic:masterfrom
nik9000 merged 5 commits intoelastic:masterfrom
Conversation
This prevent `keyword` valued runtime scripts from emitting too many values or values that take up too much space. Without this you can put allocate a ton of memory with the script by sticking it into a tight loop. Painless has some protections against this but: 1. I don't want to rely on them out of sheer paranoia 2. They don't really kick in when the script uses callbacks like we do anyway. Relates to elastic#59332
Collaborator
|
Pinging @elastic/es-search (:Search/Search) |
javanna
reviewed
Sep 4, 2020
Contributor
javanna
left a comment
There was a problem hiding this comment.
the approach looks good to me, I guess the plan is to apply it to all types and share as much code as possible?
| /** | ||
| * The maximum number of values a script should be allowed to emit. | ||
| */ | ||
| public static final int MAX_VALUES = 1000; |
| protected final void emitValue(String v) { | ||
| if (results.size() >= MAX_VALUES) { | ||
| throw new IllegalArgumentException("too many runtime values"); | ||
| } |
Contributor
There was a problem hiding this comment.
could the max values check be shared in the base class, and could we then unify the error message to something like "Runtime field [field] is emitting [1500] values while the maximum number of values allowed is [1000]"?
| } | ||
| chars += v.length(); | ||
| if (chars >= MAX_CHARS) { | ||
| throw new IllegalArgumentException("too many characters in runtime values [" + chars + "]"); |
Contributor
There was a problem hiding this comment.
Runtime field [field] is emitting [1500] characters while the maximum number of characters allowed is [1000]"?
Member
Author
|
Yes to all of those. I can try and pull it into the super class in this pr.
Do you have a preference for the max?
…On Fri, Sep 4, 2020, 05:32 Luca Cavanna ***@***.***> wrote:
***@***.**** commented on this pull request.
the approach looks good to me, I guess the plan is to apply it to all
types and share as much code as possible?
------------------------------
In
x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/AbstractScriptFieldScript.java
<#61938 (comment)>
:
> @@ -27,6 +27,11 @@
* ***@***.*** AggregationScript} but hopefully with less historical baggage.
*/
public abstract class AbstractScriptFieldScript {
+ /**
+ * The maximum number of values a script should be allowed to emit.
+ */
+ public static final int MAX_VALUES = 1000;
this seems high
------------------------------
In
x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/StringScriptFieldScript.java
<#61938 (comment)>
:
> setDocument(docId);
execute();
return results;
}
protected final void emitValue(String v) {
+ if (results.size() >= MAX_VALUES) {
+ throw new IllegalArgumentException("too many runtime values");
+ }
could the max values check be shared in the base class, and could we then
unify the error message to something like "Runtime field [field] is
emitting [1500] values while the maximum number of values allowed is
[1000]"?
------------------------------
In
x-pack/plugin/runtime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/StringScriptFieldScript.java
<#61938 (comment)>
:
> setDocument(docId);
execute();
return results;
}
protected final void emitValue(String v) {
+ if (results.size() >= MAX_VALUES) {
+ throw new IllegalArgumentException("too many runtime values");
+ }
+ chars += v.length();
+ if (chars >= MAX_CHARS) {
+ throw new IllegalArgumentException("too many characters in runtime values [" + chars + "]");
Runtime field [field] is emitting [1500] characters while the maximum
number of characters allowed is [1000]"?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#61938 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABUXIUULXYGBMEZB2G3LFDSECX2PANCNFSM4QVD42FQ>
.
|
Contributor
|
Does 100 sound reasonable for the max values? |
Member
Author
|
Sure!
…On Fri, Sep 4, 2020, 08:50 Luca Cavanna ***@***.***> wrote:
Does 100 sound reasonable for the max values?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#61938 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABUXIWJTUG2PWBPWWXD653SEDPCLANCNFSM4QVD42FQ>
.
|
Member
Author
|
@javanna this is ready for you again! |
javanna
reviewed
Sep 8, 2020
...me-fields/src/main/java/org/elasticsearch/xpack/runtimefields/AbstractScriptFieldScript.java
Show resolved
Hide resolved
30 tasks
javanna
approved these changes
Sep 9, 2020
Member
Author
|
Thanks @javanna ! |
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Sep 9, 2020
This prevent `keyword` valued runtime scripts from emitting too many values or values that take up too much space. Without this you can put allocate a ton of memory with the script by sticking it into a tight loop. Painless has some protections against this but: 1. I don't want to rely on them out of sheer paranoia 2. They don't really kick in when the script uses callbacks like we do anyway. Relates to elastic#59332
nik9000
added a commit
that referenced
this pull request
Sep 9, 2020
This prevent `keyword` valued runtime scripts from emitting too many values or values that take up too much space. Without this you can put allocate a ton of memory with the script by sticking it into a tight loop. Painless has some protections against this but: 1. I don't want to rely on them out of sheer paranoia 2. They don't really kick in when the script uses callbacks like we do anyway. Relates to #59332
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This prevent
keywordvalued runtime scripts from emitting too manyvalues or values that take up too much space. Without this you can put
allocate a ton of memory with the script by sticking it into a tight
loop. Painless has some protections against this but:
anyway.
Relates to #59332