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

draft: Memory leak fix in JVM #1854

Draft
wants to merge 1 commit into
base: community
Choose a base branch
from

Conversation

santaevpavel
Copy link

This PR fixes memory leak issues that is described here, but this fix introduces new issues.

Where is leak?

The leak was happening in JNI code that is generated by SWIG. Long and unsigned chat arrays that were used to pass java arrays to C code weren't deallocated.
SWIG_JavaArrayInLonglong allocates array of longs and that array isn't deallocated. See JNI code:

SWIGEXPORT jboolean JNICALL Java_io_realm_kotlin_internal_interop_realmcJNI_realm_1results_1count(JNIEnv *jenv, jclass jcls, jlong jarg1, jlongArray jarg2) {
  jboolean jresult = 0 ;
  realm_results_t *arg1 = (realm_results_t *) 0 ;
  size_t *arg2 = (size_t *) 0 ;
  jlong *jarr2 ;
  bool result;
  
  (void)jenv;
  (void)jcls;
  arg1 = *(realm_results_t **)&jarg1; 
  
  if(!SWIG_JavaArrayInLonglong(jenv, &jarr2, (long long **)&arg2, jarg2)) return 0;
  
  result = (bool)realm_results_count(arg1,arg2);
  {
    if (!result) {
      bool exception_thrown = throw_last_error_as_java_exception(jenv);
      if (exception_thrown) {
        // Return immediately if there was an error in which case the exception will be raised when returning to JVM
        return 0;
      }
    }
    jresult = (jboolean)result;
  }
  
  SWIG_JavaArrayArgoutLonglong(jenv, jarr2, (long long *)arg2, jarg2);
  
  return jresult;
}

Solution

I worked with SWIG first time, so I am not sure that's correct solution. At least changing %typemap(freearg) void**; to %typemap(freearg) long long**; makes SWIG generate deallocation of long array.
I also profiled the library with my fix and I confirm that it fixes leaks.

Problem

There is a least one function (realm_binary_t_data_set()) where used long array shouldn't be deallocated. It leads to crash when deallocating realm_value_t as data is freed in realm_binary_t_data_set().

Copy link

cla-bot bot commented Nov 12, 2024

Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @santaevpavel. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a @cla-bot check comment. The GitHub usernames you file there will need to match that of your Pull Requests. If you have any questions or cannot file the CLA electronically, make a comment here and we will be happy to help you out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant