Copy buffers to JS memory and free

[jamshale]

TODO: More testing of the ffi wrapper and binary. Currently testing via the compiled javascript in my project.

This copies the buffers into JS memory and frees it from rust memory. Otherwise this leads to a memory leak which can become important for heavy traffic scenarios like a mediator server under load.

Here is a snippet from allocation growth without freeing the buffers

     MB   %
      0.1   0.2% aries_askar::ffi::EnsureCallback::resolve (inline)
      0.1   0.2% aries_askar::ffi::handle::ArcHandle::create (inline)
      2.2   2.6% aries_askar::ffi::key::askar_key_aead_decrypt::{{closure}} (inline)
      2.4   2.9% aries_askar::ffi::key::askar_key_aead_encrypt::{{closure}} (inline)
      0.1   0.2% aries_askar::ffi::key::askar_key_crypto_box_random_nonce::{{closure}} (inline)
      0.3   0.3% aries_askar::ffi::key::askar_key_crypto_box_seal::{{closure}} (inline)
      0.1   0.2% aries_askar::ffi::key::askar_key_from_public_bytes::{{closure}} (inline)
      0.1   0.2% aries_askar::ffi::result_list::askar_entry_list_get_name::{{closure}} (inline)
      1.3   1.5% aries_askar::ffi::result_list::askar_entry_list_get_tags::{{closure}} (inline)
      5.0   6.1% aries_askar::ffi::result_list::askar_entry_list_get_value::{{closure}} (inline)
      0.1   0.2% aries_askar::ffi::result_list::askar_key_entry_list_get_name::{{closure}} (inline)
      2.4   2.9% aries_askar::ffi::secret::EncryptedBuffer::from_encrypted
      9.8  11.8% aries_askar::ffi::secret::SecretBuffer::from_secret (inline)
      2.4   2.9% aries_askar::ffi::store::askar_scan_next::{{closure}}::{{closure}}
      0.1   0.2% aries_askar::ffi::store::askar_scan_next::{{closure}}::{{closure}} (inline)
      2.3   2.7% aries_askar::ffi::store::askar_scan_next::{{closure}}::{{closure}}::{{closure}} (inline)
      0.3   0.3% aries_askar::kms::envelope::crypto_box_seal
      0.1   0.2% aries_askar::kms::local_key::LocalKey::from_public_bytes (inline)
      0.1   0.2% askar_crypto::alg::any::from_public_bytes_any (inline)
      4.6   5.6% askar_crypto::buffer::secret::SecretBytes::shrink_to_fit
      4.9   5.9% askar_crypto::buffer::secret::SecretBytes::with_capacity (inline)
      0.3   0.3% askar_crypto::encrypt::crypto_box::crypto_box_seal
     0.0   0.0% 100.0%      0.1   0.2% askar_entry_list_get_name

And here is roughly the same load with the buffers being freed. This did run for a longer time.

        MB    %
         0.1   1.0% aries_askar::ffi::EnsureCallback::resolve (inline)
         0.1   1.0% aries_askar::ffi::handle::ArcHandle::create (inline)
         0.1   1.0% aries_askar::ffi::key::askar_key_from_public_bytes::{{closure}} (inline)
         0.1   1.0% aries_askar::ffi::result_list::askar_entry_list_get_category::{{closure}} (inline)
         0.3   1.9% aries_askar::ffi::result_list::askar_entry_list_get_name::{{closure}} (inline)
         1.0   7.7% aries_askar::ffi::result_list::askar_entry_list_get_tags::{{closure}} (inline)
         1.4  10.6% aries_askar::ffi::store::askar_scan_next::{{closure}}::{{closure}}
         0.1   1.0% aries_askar::ffi::store::askar_scan_next::{{closure}}::{{closure}} (inline)
         1.3   9.6% aries_askar::ffi::store::askar_scan_next::{{closure}}::{{closure}}::{{closure}} (inline)
         0.1   1.0% aries_askar::kms::local_key::LocalKey::from_public_bytes (inline)
         0.1   1.0% askar_crypto::alg::any::from_public_bytes_any (inline)

The SecretBuffer::from_secret no longer show up.

Meaning the allocations are not contributing to memory growth. This prevents OOM when node is trying to use a high percentage of it's allocated memory and for this memory to be reclaimed when load stops or memory limits are being reached.

[changeset-bot]

🦋 Changeset detected

Latest commit: d1dae0e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@openwallet-foundation/askar-react-native	Patch
@openwallet-foundation/askar-nodejs	Patch
@openwallet-foundation/askar-shared	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[TimoGlastra]

Does .from copy the buffer? Is the behavior different from new Uint8Array?

How does it differ from:

Suggested change

	const view = secretBufferToBuffer(byteBuffer)
	const result = Buffer.from(view)
	this.nativeAriesAskar.askar_buffer_free(byteBuffer)
	return new Uint8Array(result)
	const result = new Uint8Array(secretBufferToBuffer(byteBuffer))
	this.nativeAriesAskar.askar_buffer_free(byteBuffer)
	return result

[jamshale]

I think you maybe right. I was doing it explicitly to understand that the buffer was getting copied into JS memory, but I should be able to do it in a single line. I'll test it out.

[TimoGlastra]

This is great @jamshale! How about the remaining memory usage? Is that normal or could there br other leaks? Like the scan?

[jamshale]

This is great @jamshale! How about the remaining memory usage? Is that normal or could there br other leaks? Like the scan?

Good point. I thought the scan and entry_list operations should be ok because the have scanFree and entryListFree functions in the ffi api that should be getting called. I'm not sure why the are showing up in the positive allocations without a corresponding negative allocation. Potentially they aren't freeing all object that they should be. I'll have a look into this.

[jamshale]

I think this should be good now. It was a bit challenging to get things lined up right because I am developing against the v0.5.x credo mediator which uses the old @hyperledger/aries-askar-nodejs libraries. I'm curious if there's a way to target those for a release with these changes or get credo v0.5.x to use the new @openwallet ones.

For the other memory allocations I think there is still some memory being leaked although the seem very small. One has to do with the StringBuffers not being freed from askar_entry_list_get_tags and similar functions. This may require exposing a function in the rust layer. The other is in askar_scan_next, it seems like the entry_list_handles are never being freed. Only the scan_handles. If I tried to free the handles the wallet had issues opening so I don't know how to deal with them currently.

These other potential leaks are small and may not need to be fixed and could be tackled in separately if someone wants to try and fix them.

[TimoGlastra]

Would it be possible to first create a PR targetting main? Then we at least have the code for the latest release track. I don't think we can backport this easily to 0.2.4 (since it's under a different scope, and would require quite some setup on our side) 🤔 I'll think a bit more about how to release this for 0.5.x.

Updating to the new askar packages would be a breaking change for 0.5.x.

[jamshale]

I'll rebase this to main.

I can probably create a patch for it until we can move the mediator to version 0.6.x

[genaris]

Would it be possible to first create a PR targetting main? Then we at least have the code for the latest release track. I don't think we can backport this easily to 0.2.4 (since it's under a different scope, and would require quite some setup on our side) 🤔 I'll think a bit more about how to release this for 0.5.x.

Updating to the new askar packages would be a breaking change for 0.5.x.

I think we'll need to find a way to get a new Askar release compatible with Credo 0.5.x, since we are very close to the deadline imposed by Android for 16KB page size support. There is a @openwallet-foundation/askar 0.2.5 released from this repo (0.2.4 branch). Does it work? Can we do something similar to that? Of course this alone will not be enough, since we'll need a backport of Askar 0.3.x as well.

[swcurran]

Should hear back from @andrewwhitehead soon about a 0.3.3 release of Askar,

[TimoGlastra]

Hmm i think it might be easier to just integrate the latest owf askar release into 0.5.x and support both askar interfaces in the 0.5.x stream. There's not a lot / any breaking api changes besides the naming ariesAskar vs askar.

By supporting both it won't be a breaking change, but it does allow 0.5.x users to move towards the new askar release

[berendsliedrecht]

Just as a small note, we are looking into replacing the wrapper to not use a deprecated library and to make it work with the latest node.js. So, I would not spend too much time on adding more and more tests for ffi-napi specific things :).

[TimoGlastra]

So, I would not spend too much time on adding more and more tests for ffi-napi specific things :).

That's fair, but we should make sure we fix the memory leaks if we switch over. So it's good to already identify the root issues (and I suspect the migration will probably take quite a while still)

[TimoGlastra]

FYI @jamshale, we're working to add the latest askar release to Credo 0.5.x. This means we won't have to backport it, but can directly merge it into the main branch, and then can be used with the next Credo 0.5.x release.

[TimoGlastra]

We're looking into the React Native side as well now since it has the same issue, and then we can get this merged

[TimoGlastra]

Okay updated the code a bit and added React Native support, this should now be ready. Probably good to test it first with the alpha release in React Native / Credo, and then we can make a new stable release