check if data size exceeds java array vm limit when it is copied in jni #3850
Conversation
code and throw RocksDBException in case of failure
java/rocksjni/portal.h
Outdated
|
|
||
| const jsize jlen = static_cast<jsize>(size); | ||
| jbyteArray jbytes = env->NewByteArray(jlen); | ||
|
|
There was a problem hiding this comment.
Are you missing the check for OutOfMemoryError here?
|
@azagrebin has updated the pull request. View: changes |
|
@azagrebin has updated the pull request. View: changes |
|
not sure why travis fails (Error 137), maybe creating of 2.2Gb value is too big for it or the unit test takes too long |
java/rocksjni/portal.h
Outdated
| static jbyteArray copyBytes(JNIEnv* env, const Slice& bytes) { | ||
| const jsize jlen = static_cast<jsize>(bytes.size()); | ||
|
|
||
| static jbyteArray createJavaByteArrayWithSizeCheck(JNIEnv* env, const char* bytes, const size_t size) { |
There was a problem hiding this comment.
Could you add some function documentation please?
In particular can you add an @throws.
I have been using the Javadoc style even though it is C++, as there are some C++ documenters that will happily read javadoc style comments from C++.
java/rocksjni/portal.h
Outdated
| // Integer.MAX_VALUE - 8 should be safe enough | ||
| static const size_t MAX_JARRAY_SIZE = ((static_cast<size_t>(1)) << 31) - 9; | ||
| if(size > MAX_JARRAY_SIZE) { | ||
| rocksdb::RocksDBExceptionJni::ThrowNew(env, "Requested array size exceeds VM limit"); |
There was a problem hiding this comment.
The though occurs to me that, we are now potentially throwing a RocksDBException for any caller of copyBytes which we were not previously.
So we need to look for each Java function that eventually calls this code and make sure it has an @throws RocksDBException on both itself and its native interface function. A quick grep -r "copyBytes" java/rocksdbjni shows that we will in the least need changes to WriteBatch.Handler functions, amongst probably others.
There was a problem hiding this comment.
As I understand, WriteBatch.Handler methods are called from JNI code which is behind WriteBatch.iterate (marked as throws RocksDBException). copyBytes is called from there before, to prepare key/value for WriteBatch.Handler. WriteBatch.Handler methods are user specific java code.
I also found couple of other places which can reach copyBytes.
|
@azagrebin Regards your Travis problem, it seems you ran out of "resources", see: travis-ci/travis-ci#4665 This document (https://docs.travis-ci.com/user/reference/overview/) seems to suggest that we have 4GB available to the build, but I am not sure how much will be used up by Rocks internally etc. I think for your JUnit test, we need to add an assumption (https://junit.org/junit4/javadoc/4.12/org/junit/Assume.html) that we have enough memory present to run your test. You could calculate the available memory (https://stackoverflow.com/questions/6220790/whats-the-best-way-to-calculate-available-memory-in-java#16517624) and check you have enough for your array in an Assumption. We could also add a test in C++ to call the |
…est if there is not enough memory for it
|
@azagrebin has updated the pull request. View: changes |
|
@adamretter There is an option to expose test JNI method which calls |
|
@adamretter |
|
@adamretter |
|
@azagrebin yes I think it should be okay to merge. @sagar0 / @gfosco Can you merge please? |
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@sagar0 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
…ni (facebook#3850) Summary: to address issue facebook#3849 Closes facebook#3850 Differential Revision: D8695487 Pulled By: sagar0 fbshipit-source-id: 04baeb2127663934ed1321fe6d9a9ec23c86e16b
to address issue #3849