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

Set maxOutputLength correctly #16604

Merged
merged 1 commit into from
Mar 18, 2023
Merged

Conversation

losipiuk
Copy link
Member

@losipiuk losipiuk commented Mar 17, 2023

Previously we were not accounting for bytesPreserved.
With recent check added to aircompressor code
(airlift/aircompressor@127b7f3)
it started manifesting as java.lang.IllegalArgumentException.

This is strictly validation fix. The passed array was not filled beyond
allocated size.

Description

fixes: #16541

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required.
(x) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Mar 17, 2023
@losipiuk losipiuk requested review from arhimondr, findepi and dain March 17, 2023 09:35
@losipiuk
Copy link
Member Author

@arhimondr now clue how to test it though for now. The TestPagesSerde is not triggering errornous condition.

@@ -337,7 +337,7 @@ private void decompress()
blockSize,
sink.getSlice().byteArray(),
sink.getSlice().byteArrayOffset() + bytesPreserved,
sink.getSlice().length());
sink.getSlice().length() - bytesPreserved);
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually code below:

System.arraycopy(
                        source.getSlice().byteArray(),
                        source.getSlice().byteArrayOffset() + source.getPosition(),
                        sink.getSlice().byteArray(),
                        sink.getSlice().byteArrayOffset() + bytesPreserved,
                        blockSize);

seems broken too. Do we know that blockSize will always fit in the targetSlice given some of the data is taken by bytesPreserved @arhimondr ?

Copy link
Contributor

Choose a reason for hiding this comment

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

int bufferSize = blockSizeInBytes
                        // to guarantee a single long can always be read entirely
                        + Long.BYTES;
                buffers[0] = new ReadBuffer(Slices.allocate(bufferSize))

The buffer size is allocated according to blockSize with an extra 8 bytes (one long) for carry-overs.

The carry over is guaranteed to never be higher than 8 bytes as this is the most what can be requested to be read in one shot.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@losipiuk losipiuk force-pushed the lo/fix-lzo-buffer branch from e671f64 to d0898ef Compare March 17, 2023 22:14
@losipiuk
Copy link
Member Author

@arhimondr I tried to add regression test (attempt is here) but with no luck. I cannot trigger the code flow which results in check failure.
Unless you have some idea how to proceed with test lest merge it as is.

Previously we were not accounting for bytesPreserved.
With recent check added to aircompressor code
(airlift/aircompressor@127b7f3)
it started manifesting as java.lang.IllegalArgumentException.

This is strictly validation fix. The passed array was not filled beyond
allocated size.
@losipiuk losipiuk force-pushed the lo/fix-lzo-buffer branch from d0898ef to f50081c Compare March 18, 2023 13:42
@losipiuk
Copy link
Member Author

@arhimondr updated with test

Copy link
Contributor

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

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

Thanks. Great idea with constructing a specialized block encoder to cover the problematic code path. Love it!

@losipiuk losipiuk merged commit 03e24ec into trinodb:master Mar 18, 2023
@github-actions github-actions bot added this to the 411 milestone Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

IllegalArgumentException when running INSERT INTO SELECT after updating to 410
2 participants