Skip to content

Add a workaround for empty Slice copy SIGSEGV#12850

Closed
lukasz-stec wants to merge 1 commit intotrinodb:masterfrom
starburstdata:ls/022-spa-empty-slice-sigsegv
Closed

Add a workaround for empty Slice copy SIGSEGV#12850
lukasz-stec wants to merge 1 commit intotrinodb:masterfrom
starburstdata:ls/022-spa-empty-slice-sigsegv

Conversation

@lukasz-stec
Copy link
Copy Markdown
Member

Description

Due to jvm bug, compiled code could fail
with SIGSEGV when copying 0 bytes from an empty Slice.
This change avoids copy attempts altogether, which
additionally increases performance in empty Slice case.

Is this change a fix, improvement, new feature, refactoring, or other?

fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

core query engine

How would you describe this change to a non-technical end user or system administrator?

Avoid worker process crash when using vulnerable JVM version

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

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

Due to jvm bug, compiled code could fail
with SIGSEGV when copying 0 bytes from an empty Slice.
This change avoids copy attempt altogether, which
additionally increases performance in empty Slice case.
@cla-bot cla-bot bot added the cla-signed label Jun 15, 2022
@lukasz-stec lukasz-stec requested a review from sopel39 June 15, 2022 07:42
@findepi findepi requested a review from martint June 15, 2022 09:44
@findepi
Copy link
Copy Markdown
Member

findepi commented Jun 15, 2022

@martint offered a different fix: #12841

@lukasz-stec
Copy link
Copy Markdown
Member Author

@martint offered a different fix: #12841

This change may also be considered a performance improvement (avoiding unnecessary work)

@electrum
Copy link
Copy Markdown
Member

Per the Javadoc for Unsafe:

Note: It is the responsibility of the caller to make sure arguments are checked before the methods are called. While some rudimentary checks are performed on the input, the checks are best effort and when performance is an overriding priority, as when methods of this class are optimized by the runtime compiler, some or all checks (if any) may be elided. Hence, the caller must not rely on the checks and corresponding exceptions!

However, I can't see an argument that copying zero bytes should ever try to read from the source, so this does indeed seem like a bug in the JVM.

@martint
Copy link
Copy Markdown
Member

martint commented Jun 17, 2022

#12841 is merged, so this workaround shouldn't be needed anymore.

@lukasz-stec
Copy link
Copy Markdown
Member Author

#12841 is merged, so this workaround shouldn't be needed anymore.

let's close it.

@raunaqmorarka
Copy link
Copy Markdown
Member

@lukasz-stec could we keep the test case from this PR to detect this problem more easily in case it reoccurs in future ?

@lukasz-stec
Copy link
Copy Markdown
Member Author

@lukasz-stec could we keep the test case from this PR to detect this problem more easily in case it reoccurs in future ?

@raunaqmorarka created #12927

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.

JVM crashing with SIGSEGV due to union query

5 participants