Skip to content

Conversation

@s1monw
Copy link
Contributor

@s1monw s1monw commented Sep 18, 2017

Today we can't validate the array length in InputStreamStreamInput since
we can't rely on InputStream.available yet in some situations we know
the size of the stream and can apply additional validation.

Today we can't validate the array length in `InputStreamStreamInput` since
we can't rely on `InputStream.available` yet in some situations we know
the size of the stream and can apply additional validation.
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice.

private final InputStream is;
private final long sizeLimit;

public InputStreamStreamInput(InputStream is) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth adding javadoc here too.

try (InputStream in = new ByteArrayInputStream(qbSource.bytes, qbSource.offset, qbSource.length)) {
try (StreamInput input = new NamedWriteableAwareStreamInput(new InputStreamStreamInput(in), registry)) {
try (StreamInput input = new NamedWriteableAwareStreamInput(new InputStreamStreamInput(in, qbSource.length),
registry)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change the line wrapping on this somehow? Like stick new InputStreamStreamInput on a new line and indent it? I think as is it'd break how I visually scan try-with-resources.

try (StreamInput input = new NamedWriteableAwareStreamInput(new InputStreamStreamInput(in, qbSource.length),
registry)) {
try (StreamInput input = new NamedWriteableAwareStreamInput(
new InputStreamStreamInput(in, qbSource.length), registry)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@s1monw s1monw merged commit 9f97f90 into elastic:master Sep 18, 2017
@s1monw s1monw deleted the allow_input_stream_validation branch September 18, 2017 15:52
s1monw added a commit that referenced this pull request Sep 18, 2017
…#26692)

Today we can't validate the array length in `InputStreamStreamInput` since
we can't rely on `InputStream.available` yet in some situations we know
the size of the stream and can apply additional validation.
s1monw added a commit that referenced this pull request Sep 18, 2017
…#26692)

Today we can't validate the array length in `InputStreamStreamInput` since
we can't rely on `InputStream.available` yet in some situations we know
the size of the stream and can apply additional validation.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 19, 2017
* master:
  added comment
  fix line length violation
  Moved the check to fetch phase. This basically means that we throw a better error message instead of an AOBE and not adding more restrictions.
  inner hits: Do not allow inner hits that use _source and have a non nested object field as parent
  Separate Painless Whitelist Loading from the Painless Definition (elastic#26540)
  convert more admin requests to writeable (elastic#26566)
  Handle release of 5.6.1
  Allow `InputStreamStreamInput` array size validation where applicable (elastic#26692)
  Update global checkpoint with permit after recovery
jasontedor added a commit to droberts195/elasticsearch that referenced this pull request Sep 19, 2017
* master: (278 commits)
  Move pre-6.0 node checkpoint to SequenceNumbers
  Invalid JSON request body caused endless loop (elastic#26680)
  added comment
  fix line length violation
  Moved the check to fetch phase. This basically means that we throw a better error message instead of an AOBE and not adding more restrictions.
  inner hits: Do not allow inner hits that use _source and have a non nested object field as parent
  Separate Painless Whitelist Loading from the Painless Definition (elastic#26540)
  convert more admin requests to writeable (elastic#26566)
  Handle release of 5.6.1
  Allow `InputStreamStreamInput` array size validation where applicable (elastic#26692)
  Update global checkpoint with permit after recovery
  Filter pre-6.0 nodes for checkpoint invariants
  Skip bad request REST test on pre-6.0
  Reenable BWC tests after disabling for backport
  Add global checkpoint tracking on the primary
  [Test] Fix reference/cat/allocation/line_8 test failure
  [Docs] improved description for fs.total.available_in_bytes (elastic#26657)
  Fix discovery-file plugin to use custom config path
  fix testSniffNodes to use the new error message
  Add check for invalid index in WildcardExpressionResolver (elastic#26409)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 20, 2017
* master: (67 commits)
  Restoring from snapshot should force generation of a new history uuid (elastic#26694)
  test: Use a single primary shard so that the exception can caught in the same way
  Move pre-6.0 node checkpoint to SequenceNumbers
  Invalid JSON request body caused endless loop (elastic#26680)
  added comment
  fix line length violation
  Moved the check to fetch phase. This basically means that we throw a better error message instead of an AOBE and not adding more restrictions.
  inner hits: Do not allow inner hits that use _source and have a non nested object field as parent
  Separate Painless Whitelist Loading from the Painless Definition (elastic#26540)
  convert more admin requests to writeable (elastic#26566)
  Handle release of 5.6.1
  Allow `InputStreamStreamInput` array size validation where applicable (elastic#26692)
  Update global checkpoint with permit after recovery
  Filter pre-6.0 nodes for checkpoint invariants
  Skip bad request REST test on pre-6.0
  Reenable BWC tests after disabling for backport
  Add global checkpoint tracking on the primary
  [Test] Fix reference/cat/allocation/line_8 test failure
  [Docs] improved description for fs.total.available_in_bytes (elastic#26657)
  Fix discovery-file plugin to use custom config path
  ...
@lcawl lcawl removed the v6.1.0 label Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants